Only skip ToS for fully managed devices.

Bug: 1108553
Change-Id: Ifa4f7d45a45ebcc8209cf0b1a83da25089a8fd5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363881
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799805}

chrome/android/java/src/org/chromium/chrome/browser/firstrun/TosAndUmaFirstRunFragmentWithEnterpriseSupport.java

@@ -8,11 +8,11 @@ import android.os.Bundle;
 import android.view.View;
 
 import androidx.annotation.Nullable;
-import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.CallbackController;
 import org.chromium.base.Log;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.policy.EnterpriseInfo;
 import org.chromium.chrome.browser.policy.PolicyServiceFactory;
 import org.chromium.components.browser_ui.widget.LoadingView;
 import org.chromium.policy.PolicyService;
@@ -56,12 +56,20 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
      * should skip the rest of FRE. This can be null when this information is not ready yet.
      */
     private @Nullable Boolean mPolicyCctTosDialogEnabled;
-
-    private static boolean sBlockPolicyLoadingForTest;
+    /**
+     * Whether the current device is organization owned. This will start null before the check
+     * occurs. The FRE can only be skipped if the device is owned corporate owned.
+     */
+    private @Nullable Boolean mIsDeviceOwned;
 
     private TosAndUmaFirstRunFragmentWithEnterpriseSupport() {
         mCallbackController = new CallbackController();
         checkAppRestriction();
+        // It's possible for app restrictions to have its callback synchronously invoked and we can
+        // give up on the skip scenario.
+        if (shouldWaitForPolicyLoading()) {
+            checkIsDeviceOwned();
+        }
     }
 
     @Override
@@ -114,7 +122,7 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
 
     @Override
     public void onHideLoadingUIComplete() {
-        if (confirmedCctTosDialogDisabled()) {
+        if (confirmedCctTosDialogDisabled() && confirmedOwnedDevice()) {
             // TODO(crbug.com/1108564): Show the different UI that has the enterprise disclosure.
             exitCctFirstRun();
         } else {
@@ -130,7 +138,14 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
      *         and can update the UI immediately.
      */
     private boolean shouldWaitForPolicyLoading() {
-        return !confirmedNoAppRestriction() && mPolicyCctTosDialogEnabled == null;
+        // Note that someSignalOutstanding doesn't care about mHasRestriction. It's main purpose is
+        // to be a very quick signal mPolicyCctTosDialogEnabled is never going to turn false. But
+        // once mPolicyCctTosDialogEnabled has a non-null value, mHasRestriction is redundant. It
+        // never actually needs to return for us to know we can skip the ToS.
+        boolean someSignalOutstanding =
+                mPolicyCctTosDialogEnabled == null || mIsDeviceOwned == null;
+        boolean mightStillBeAllowedToSkip = !confirmedToShowUmaAndTos();
+        return someSignalOutstanding && mightStillBeAllowedToSkip;
     }
 
     /**
@@ -140,7 +155,8 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
      * @return Whether we should show TosAndUma components on the UI.
      */
     private boolean confirmedToShowUmaAndTos() {
-        return confirmedNoAppRestriction() || confirmedCctTosDialogEnabled();
+        return confirmedNoAppRestriction() || confirmedCctTosDialogEnabled()
+                || confirmedNotOwnedDevice();
     }
 
     private boolean confirmedNoAppRestriction() {
@@ -155,16 +171,32 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
         return mPolicyCctTosDialogEnabled != null && !mPolicyCctTosDialogEnabled;
     }
 
+    private boolean confirmedNotOwnedDevice() {
+        return mIsDeviceOwned != null && !mIsDeviceOwned;
+    }
+
+    private boolean confirmedOwnedDevice() {
+        return mIsDeviceOwned != null && mIsDeviceOwned;
+    }
+
     private void checkAppRestriction() {
         FirstRunAppRestrictionInfo.getInstance().getHasAppRestriction(
                 mCallbackController.makeCancelable(this::onAppRestrictionDetected));
     }
 
     private void onAppRestrictionDetected(boolean hasAppRestriction) {
+        // It's possible that we've already told the spinner to hide, and even signaled to our
+        // delegate to exit. If so, we can ignore the app restrictions value.
+        // TODO(https://crbug.com/1119449): Shouldn't need this check if we can cancel this callback
+        // when we no longer need it.
+        if (!shouldWaitForPolicyLoading()) {
+            return;
+        }
+
         mHasRestriction = hasAppRestriction;
 
         if (!shouldWaitForPolicyLoading() && mViewCreated) {
-            // TODO(crbug.com/1106812): Unregister policy listener.
+            // TODO(https://crbug.com/1119449): Cleanup various policy callbacks.
             mLoadingSpinner.hideLoadingUI();
         }
     }
@@ -185,14 +217,28 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
 
     private void updateCctTosPolicy() {
         mPolicyCctTosDialogEnabled = FirstRunUtils.isCctTosDialogEnabled();
-        if (mViewCreated) {
+        if (!shouldWaitForPolicyLoading() && mViewCreated) {
+            // TODO(https://crbug.com/1119449): Cleanup various policy callbacks.
+            mLoadingSpinner.hideLoadingUI();
+        }
+    }
+
+    private void checkIsDeviceOwned() {
+        EnterpriseInfo.getInstance().getDeviceEnterpriseInfo(
+                mCallbackController.makeCancelable(this::onIsDeviceOwnedDetected));
+    }
+
+    private void onIsDeviceOwnedDetected(EnterpriseInfo.OwnedState ownedState) {
+        mIsDeviceOwned = ownedState.mDeviceOwned;
+        if (!shouldWaitForPolicyLoading() && mViewCreated) {
+            // TODO(https://crbug.com/1119449): Cleanup various policy callbacks.
             mLoadingSpinner.hideLoadingUI();
         }
     }
 
-    @VisibleForTesting
-    void exitCctFirstRun() {
+    private void exitCctFirstRun() {
         assert confirmedCctTosDialogDisabled();
+        assert confirmedOwnedDevice();
         // TODO(crbug.com/1108564): Fire a signal to end this fragment when disclaimer is ready.
         // TODO(crbug.com/1108582): Save a shared pref indicating Enterprise CCT FRE is complete,
         //  and skip waiting for future cold starts.

chrome/android/java/src/org/chromium/chrome/browser/policy/EnterpriseInfo.java

@@ -29,7 +29,7 @@ import java.util.concurrent.RejectedExecutionException;
 /**
  * Provide the enterprise information for the current device and profile.
  */
-public final class EnterpriseInfo {
+public class EnterpriseInfo {
     private static final String TAG = "EnterpriseInfo";
 
     private static EnterpriseInfo sInstance;
@@ -40,9 +40,10 @@ public final class EnterpriseInfo {
 
     private boolean mSkipAsyncCheckForTesting;
 
-    static class OwnedState {
-        final boolean mDeviceOwned;
-        final boolean mProfileOwned;
+    /** A simple tuple to hold onto named fields about the state of ownership. */
+    public static class OwnedState {
+        public final boolean mDeviceOwned;
+        public final boolean mProfileOwned;
 
         public OwnedState(boolean isDeviceOwned, boolean isProfileOwned) {
             mDeviceOwned = isDeviceOwned;
@@ -70,6 +71,11 @@ public final class EnterpriseInfo {
         return sInstance;
     }
 
+    @VisibleForTesting
+    public static void setInstanceForTest(EnterpriseInfo instance) {
+        sInstance = instance;
+    }
+
     /**
      * Returns, via callback, whether the device has a device owner or a profile owner.
      */

chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java

@@ -41,6 +41,7 @@ import org.chromium.chrome.browser.document.ChromeLauncherActivity;
 import org.chromium.chrome.browser.locale.DefaultSearchEngineDialogHelperUtils;
 import org.chromium.chrome.browser.locale.LocaleManager;
 import org.chromium.chrome.browser.locale.LocaleManager.SearchEnginePromoType;
+import org.chromium.chrome.browser.policy.EnterpriseInfo;
 import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.MultiActivityTestRule;
@@ -66,6 +67,8 @@ public class FirstRunIntegrationTest {
 
     @Mock
     public FirstRunAppRestrictionInfo mMockAppRestrictionInfo;
+    @Mock
+    public EnterpriseInfo mEntepriseInfo;
 
     private final Set<Class> mSupportedActivities =
             CollectionUtil.newHashSet(ChromeLauncherActivity.class, FirstRunActivity.class,
@@ -95,6 +98,7 @@ public class FirstRunIntegrationTest {
     public void tearDown() {
         FirstRunActivity.setEnableEnterpriseCCTForTest(false);
         FirstRunAppRestrictionInfo.setInstanceForTest(null);
+        EnterpriseInfo.setInstanceForTest(null);
         if (mLastActivity != null) mLastActivity.finish();
     }
 
@@ -123,6 +127,17 @@ public class FirstRunIntegrationTest {
         FirstRunAppRestrictionInfo.setInstanceForTest(mMockAppRestrictionInfo);
     }
 
+    private void setDeviceOwnedForMock() {
+        Mockito.doAnswer(invocation -> {
+                   Callback<EnterpriseInfo.OwnedState> callback = invocation.getArgument(0);
+                   callback.onResult(new EnterpriseInfo.OwnedState(true, false));
+                   return null;
+               })
+                .when(mEntepriseInfo)
+                .getDeviceEnterpriseInfo(any());
+        EnterpriseInfo.setInstanceForTest(mEntepriseInfo);
+    }
+
     @Test
     @SmallTest
     public void testHelpPageSkipsFirstRun() {
@@ -259,6 +274,7 @@ public class FirstRunIntegrationTest {
         Bundle restrictions = new Bundle();
         restrictions.putBoolean("CCTToSDialogEnabled", false);
         AbstractAppRestrictionsProvider.setTestRestrictions(restrictions);
+        setDeviceOwnedForMock();
 
         Intent intent =
                 CustomTabsTestUtils.createMinimalCustomTabIntent(mContext, "https://test.com");

chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/TosAndUmaFirstRunFragmentWithEnterpriseSupportTest.java

@@ -11,6 +11,7 @@ import android.app.Instrumentation;
 import android.app.Instrumentation.ActivityMonitor;
 import android.content.Context;
 import android.content.Intent;
+import android.os.Bundle;
 import android.support.test.InstrumentationRegistry;
 import android.view.View;
 
@@ -33,6 +34,7 @@ import org.chromium.base.CommandLine;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.customtabs.CustomTabsTestUtils;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
+import org.chromium.chrome.browser.policy.EnterpriseInfo;
 import org.chromium.chrome.browser.policy.PolicyServiceFactory;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.content_public.browser.test.util.Criteria;
@@ -70,12 +72,15 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
     public PolicyService mPolicyService;
     @Mock
     public FirstRunUtils.Natives mFirstRunUtils;
+    @Mock
+    public EnterpriseInfo mMockEnterpriseInfo;
 
-    private FirstRunActivityTestObserver mTestObserver = new FirstRunActivityTestObserver();
     private FirstRunActivity mActivity;
-    private TosAndUmaFirstRunFragmentWithEnterpriseSupport mFragment;
     private final List<PolicyService.Observer> mPolicyServiceObservers = new ArrayList<>();
     private final List<Callback<Boolean>> mAppRestrictonsCallbacks = new ArrayList<>();
+    private final List<Callback<EnterpriseInfo.OwnedState>> mOwnedStateCallbacks =
+            new ArrayList<>();
+    private int mExitCount;
 
     private View mTosText;
     private View mAcceptButton;
@@ -92,6 +97,36 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         FirstRunAppRestrictionInfo.setInstanceForTest(mMockAppRestrictionInfo);
         PolicyServiceFactory.setPolicyServiceForTest(mPolicyService);
         FirstRunUtilsJni.TEST_HOOKS.setInstanceForTesting(mFirstRunUtils);
+        EnterpriseInfo.setInstanceForTest(mMockEnterpriseInfo);
+
+        setAppRestrictionsMockNotInitialized();
+        setPolicyServiceMockNotInitialized();
+        setEnterpriseInfoNotInitialized();
+
+        // TODO(https://crbug.com/1113229): Rework this to not depend on {@link FirstRunActivity}
+        // implementation details.
+        mExitCount = 0;
+        FirstRunActivity.setObserverForTest(new FirstRunActivity.FirstRunActivityObserver() {
+            @Override
+            public void onFlowIsKnown(Bundle freProperties) {}
+
+            @Override
+            public void onAcceptTermsOfService() {}
+
+            @Override
+            public void onJumpToPage(int position) {}
+
+            @Override
+            public void onUpdateCachedEngineName() {}
+
+            @Override
+            public void onAbortFirstRunExperience() {}
+
+            @Override
+            public void onExitFirstRun() {
+                mExitCount++;
+            }
+        });
     }
 
     @After
@@ -100,56 +135,92 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         FirstRunAppRestrictionInfo.setInstanceForTest(null);
         PolicyServiceFactory.setPolicyServiceForTest(null);
         FirstRunUtilsJni.TEST_HOOKS.setInstanceForTesting(mFirstRunUtils);
+        EnterpriseInfo.setInstanceForTest(null);
         if (mActivity != null) mActivity.finish();
     }
 
     @Test
     @SmallTest
     public void testNoRestriction() {
-        setAppRestrictiosnMockNotInitialized();
         launchFirstRunThroughCustomTab();
         assertUIState(FragmentState.LOADING);
 
-        TestThreadUtils.runOnUiThreadBlocking(() -> setAppRestrictiosnMockInitialized(false));
+        setAppRestrictiosnMockInitialized(false);
         assertUIState(FragmentState.NO_POLICY);
     }
 
     @Test
     @SmallTest
-    public void testWithRestriction_DialogEnabled() {
+    public void testDialogEnabled() {
         setAppRestrictiosnMockInitialized(true);
-        setPolicyServiceMockNotInitialized();
         launchFirstRunThroughCustomTab();
         assertUIState(FragmentState.LOADING);
 
-        setMockCctTosDialogEnabled(true);
-        TestThreadUtils.runOnUiThreadBlocking(() -> setPolicyServiceMockInitialized());
+        setPolicyServiceMockInitializedWithDialogEnabled(true);
         assertUIState(FragmentState.NO_POLICY);
     }
 
     @Test
     @SmallTest
-    public void testWithRestriction_DialogDisabled() {
+    public void testNotOwnedDevice() {
         setAppRestrictiosnMockInitialized(true);
-        setPolicyServiceMockNotInitialized();
         launchFirstRunThroughCustomTab();
         assertUIState(FragmentState.LOADING);
 
-        setMockCctTosDialogEnabled(false);
-        TestThreadUtils.runOnUiThreadBlocking(() -> setPolicyServiceMockInitialized());
+        setEnterpriseInfoInitializedWithDeviceOwner(false);
+        assertUIState(FragmentState.NO_POLICY);
+    }
 
+    @Test
+    @SmallTest
+    public void testSkip_DeviceOwnedThenDialogPolicy() {
+        setAppRestrictiosnMockInitialized(true);
+        launchFirstRunThroughCustomTab();
+        assertUIState(FragmentState.LOADING);
+
+        setEnterpriseInfoInitializedWithDeviceOwner(true);
+        assertUIState(FragmentState.LOADING);
+
+        setPolicyServiceMockInitializedWithDialogEnabled(false);
+        assertUIState(FragmentState.HAS_POLICY);
+    }
+
+    @Test
+    @SmallTest
+    public void testSkip_DialogPolicyThenDeviceOwned() {
+        setAppRestrictiosnMockInitialized(true);
+        launchFirstRunThroughCustomTab();
+        assertUIState(FragmentState.LOADING);
+
+        setPolicyServiceMockInitializedWithDialogEnabled(false);
+        assertUIState(FragmentState.LOADING);
+
+        setEnterpriseInfoInitializedWithDeviceOwner(true);
+        assertUIState(FragmentState.HAS_POLICY);
+    }
+
+    @Test
+    @SmallTest
+    public void testSkip_LateAppRestrictions() {
+        launchFirstRunThroughCustomTab();
+        assertUIState(FragmentState.LOADING);
+
+        setPolicyServiceMockInitializedWithDialogEnabled(false);
+        assertUIState(FragmentState.LOADING);
+
+        // Skip should happen without app restrictions being completed.
+        setEnterpriseInfoInitializedWithDeviceOwner(true);
+        assertUIState(FragmentState.HAS_POLICY);
+
+        // assertUIState will verify that exit was not called a second time.
+        setAppRestrictiosnMockInitialized(true);
         assertUIState(FragmentState.HAS_POLICY);
-        // TODO(https://crbug.com/1113229): Rework this to not depend on {@link FirstRunActivity}
-        // implementation details.
-        Assert.assertTrue(FirstRunStatus.isEphemeralSkipFirstRun());
     }
 
     /**
      * Launch chrome through custom tab and trigger first run.
      */
     private void launchFirstRunThroughCustomTab() {
-        FirstRunActivity.setObserverForTest(mTestObserver);
-
         final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
         final Context context = instrumentation.getTargetContext();
 
@@ -172,13 +243,8 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         instrumentation.removeMonitor(freMonitor);
 
         mActivity = (FirstRunActivity) activity;
-
         CriteriaHelper.pollUiThread(
                 () -> mActivity.getSupportFragmentManager().getFragments().size() > 0);
-        mFragment = (TosAndUmaFirstRunFragmentWithEnterpriseSupport) mActivity
-                            .getSupportFragmentManager()
-                            .getFragments()
-                            .get(0);
 
         // Force this to happen now to try to make the tests more deterministic. Ideally the tests
         // could control when this happens and test for difference sequences.
@@ -204,6 +270,9 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 tosVisibility, mTosText.getVisibility());
         Assert.assertEquals("Visibility of accept button is different than the test setting.",
                 tosVisibility, mAcceptButton.getVisibility());
+
+        int expectedExitCount = fragmentState == FragmentState.HAS_POLICY ? 1 : 0;
+        Assert.assertEquals(expectedExitCount, mExitCount);
     }
 
     private void waitUntilNativeLoaded() {
@@ -211,7 +280,7 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 (() -> mActivity.isNativeSideIsInitializedForTest()), "native never initialized.");
     }
 
-    private void setAppRestrictiosnMockNotInitialized() {
+    private void setAppRestrictionsMockNotInitialized() {
         Mockito.doAnswer(invocation -> {
                    Callback<Boolean> callback = invocation.getArgument(0);
                    mAppRestrictonsCallbacks.add(callback);
@@ -230,9 +299,11 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 .when(mMockAppRestrictionInfo)
                 .getHasAppRestriction(any());
 
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
             for (Callback<Boolean> callback : mAppRestrictonsCallbacks) {
                 callback.onResult(hasAppRestrictons);
             }
+        });
     }
 
     private void setPolicyServiceMockNotInitialized() {
@@ -246,15 +317,46 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 .addObserver(any());
     }
 
-    private void setPolicyServiceMockInitialized() {
+    private void setPolicyServiceMockInitializedWithDialogEnabled(boolean cctTosDialogEnabled) {
+        setMockCctTosDialogEnabled(cctTosDialogEnabled);
+
         Mockito.when(mPolicyService.isInitializationComplete()).thenReturn(true);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
             for (PolicyService.Observer observer : mPolicyServiceObservers) {
                 observer.onPolicyServiceInitialized();
             }
+        });
         mPolicyServiceObservers.clear();
     }
 
     private void setMockCctTosDialogEnabled(boolean cctTosDialogEnabled) {
         Mockito.when(mFirstRunUtils.getCctTosDialogEnabled()).thenReturn(cctTosDialogEnabled);
     }
+
+    private void setEnterpriseInfoNotInitialized() {
+        Mockito.doAnswer(invocation -> {
+                   Callback<EnterpriseInfo.OwnedState> callback = invocation.getArgument(0);
+                   mOwnedStateCallbacks.add(callback);
+                   return null;
+               })
+                .when(mMockEnterpriseInfo)
+                .getDeviceEnterpriseInfo(any());
+    }
+
+    private void setEnterpriseInfoInitializedWithDeviceOwner(boolean hasDeviceOwner) {
+        EnterpriseInfo.OwnedState ownedState = new EnterpriseInfo.OwnedState(hasDeviceOwner, false);
+        Mockito.doAnswer(invocation -> {
+                   Callback<EnterpriseInfo.OwnedState> callback = invocation.getArgument(0);
+                   callback.onResult(ownedState);
+                   return null;
+               })
+                .when(mMockEnterpriseInfo)
+                .getDeviceEnterpriseInfo(any());
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            for (Callback<EnterpriseInfo.OwnedState> callback : mOwnedStateCallbacks) {
+                callback.onResult(ownedState);
+            }
+        });
+    }
 }