Merge "Remove NetworkMonitor dependency on ICaptivePortal"

11 files changed, 83 insertions, 53 deletions

diff --git a/Android.bp b/Android.bp
index 45a3d605f9b0..5935bfb57213 100644
--- a/Android.bp
+++ b/Android.bp
@@ -186,6 +186,7 @@ java_defaults {
         "core/java/android/hardware/radio/ITunerCallback.aidl",
         "core/java/android/hardware/soundtrigger/IRecognitionStatusCallback.aidl",
         "core/java/android/hardware/usb/IUsbManager.aidl",
+        "core/java/android/net/ICaptivePortal.aidl",
         "core/java/android/net/IConnectivityManager.aidl",
         "core/java/android/net/IIpConnectivityMetrics.aidl",
         "core/java/android/net/IEthernetManager.aidl",
@@ -815,7 +816,6 @@ aidl_interface {
     srcs: [
         "core/java/android/net/ApfCapabilitiesParcelable.aidl",
         "core/java/android/net/DhcpResultsParcelable.aidl",
-        "core/java/android/net/ICaptivePortal.aidl",
         "core/java/android/net/INetworkMonitor.aidl",
         "core/java/android/net/INetworkMonitorCallbacks.aidl",
         "core/java/android/net/IIpMemoryStore.aidl",
diff --git a/api/system-current.txt b/api/system-current.txt
index 33cc4162ca54..8efc3de8b4b9 100644
--- a/api/system-current.txt
+++ b/api/system-current.txt
@@ -3101,7 +3101,7 @@ package android.net {
     method @RequiresPermission(android.Manifest.permission.TETHER_PRIVILEGED) public void getLatestTetheringEntitlementValue(int, boolean, @NonNull android.net.ConnectivityManager.TetheringEntitlementValueListener, @Nullable android.os.Handler);
     method @RequiresPermission(anyOf={android.Manifest.permission.TETHER_PRIVILEGED, android.Manifest.permission.WRITE_SETTINGS}) public boolean isTetheringSupported();
     method @RequiresPermission(anyOf={"android.permission.NETWORK_SETTINGS", android.Manifest.permission.NETWORK_SETUP_WIZARD, "android.permission.NETWORK_STACK"}) public void setAirplaneMode(boolean);
-    method @RequiresPermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) public void startCaptivePortalApp(android.os.Bundle);
+    method @RequiresPermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) public void startCaptivePortalApp(android.net.Network, android.os.Bundle);
     method @RequiresPermission(android.Manifest.permission.TETHER_PRIVILEGED) public void startTethering(int, boolean, android.net.ConnectivityManager.OnStartTetheringCallback);
     method @RequiresPermission(android.Manifest.permission.TETHER_PRIVILEGED) public void startTethering(int, boolean, android.net.ConnectivityManager.OnStartTetheringCallback, android.os.Handler);
     method @RequiresPermission(android.Manifest.permission.TETHER_PRIVILEGED) public void stopTethering(int);
diff --git a/api/test-current.txt b/api/test-current.txt
index 83119750df41..c9d176979a65 100644
--- a/api/test-current.txt
+++ b/api/test-current.txt
@@ -608,7 +608,7 @@ package android.net {
   }
 
   public class ConnectivityManager {
-    method @RequiresPermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) public void startCaptivePortalApp(android.os.Bundle);
+    method @RequiresPermission(android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) public void startCaptivePortalApp(android.net.Network, android.os.Bundle);
     field public static final String EXTRA_CAPTIVE_PORTAL_PROBE_SPEC = "android.net.extra.CAPTIVE_PORTAL_PROBE_SPEC";
     field public static final String EXTRA_CAPTIVE_PORTAL_USER_AGENT = "android.net.extra.CAPTIVE_PORTAL_USER_AGENT";
   }
diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java
index 48f611d52c07..92b30a440d69 100644
--- a/core/java/android/net/ConnectivityManager.java
+++ b/core/java/android/net/ConnectivityManager.java
@@ -3920,15 +3920,16 @@ public class ConnectivityManager {
      *
      * <p>This endpoint is exclusively for use by the NetworkStack and is protected by the
      * corresponding permission.
+     * @param network Network on which the captive portal was detected.
      * @param appExtras Extras to include in the app start intent.
      * @hide
      */
     @SystemApi
     @TestApi
     @RequiresPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK)
-    public void startCaptivePortalApp(Bundle appExtras) {
+    public void startCaptivePortalApp(Network network, Bundle appExtras) {
         try {
-            mService.startCaptivePortalAppInternal(appExtras);
+            mService.startCaptivePortalAppInternal(network, appExtras);
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl
index 92a5839ac2af..83bb3a0c8c15 100644
--- a/core/java/android/net/IConnectivityManager.aidl
+++ b/core/java/android/net/IConnectivityManager.aidl
@@ -168,7 +168,7 @@ interface IConnectivityManager
     void setAcceptUnvalidated(in Network network, boolean accept, boolean always);
     void setAvoidUnvalidated(in Network network);
     void startCaptivePortalApp(in Network network);
-    void startCaptivePortalAppInternal(in Bundle appExtras);
+    void startCaptivePortalAppInternal(in Network network, in Bundle appExtras);
 
     boolean getAvoidBadWifi();
     int getMultipathPreference(in Network Network);
diff --git a/core/java/android/net/INetworkMonitor.aidl b/core/java/android/net/INetworkMonitor.aidl
index 41f969acad6f..c94cdde15aa8 100644
--- a/core/java/android/net/INetworkMonitor.aidl
+++ b/core/java/android/net/INetworkMonitor.aidl
@@ -34,6 +34,7 @@ oneway interface INetworkMonitor {
 
     void start();
     void launchCaptivePortalApp();
+    void notifyCaptivePortalAppFinished(int response);
     void forceReevaluation(int uid);
     void notifyPrivateDnsChanged(in PrivateDnsConfigParcel config);
     void notifyDnsResponse(int returnCode);
diff --git a/core/java/android/net/INetworkMonitorCallbacks.aidl b/core/java/android/net/INetworkMonitorCallbacks.aidl
index 514658549651..2c61511feb72 100644
--- a/core/java/android/net/INetworkMonitorCallbacks.aidl
+++ b/core/java/android/net/INetworkMonitorCallbacks.aidl
@@ -26,5 +26,4 @@ oneway interface INetworkMonitorCallbacks {
     void notifyPrivateDnsConfigResolved(in PrivateDnsConfigParcel config);
     void showProvisioningNotification(String action, String packageName);
     void hideProvisioningNotification();
-    void logCaptivePortalLoginEvent(int eventId, String packageName);
 }
\ No newline at end of file
diff --git a/packages/NetworkStack/src/com/android/server/NetworkStackService.java b/packages/NetworkStack/src/com/android/server/NetworkStackService.java
index c6a207f26577..90db207c9902 100644
--- a/packages/NetworkStack/src/com/android/server/NetworkStackService.java
+++ b/packages/NetworkStack/src/com/android/server/NetworkStackService.java
@@ -247,6 +247,12 @@ public class NetworkStackService extends Service {
         }
 
         @Override
+        public void notifyCaptivePortalAppFinished(int response) {
+            checkNetworkStackCallingPermission();
+            mNm.notifyCaptivePortalAppFinished(response);
+        }
+
+        @Override
         public void forceReevaluation(int uid) {
             checkNetworkStackCallingPermission();
             mNm.forceReevaluation(uid);
diff --git a/packages/NetworkStack/src/com/android/server/connectivity/NetworkMonitor.java b/packages/NetworkStack/src/com/android/server/connectivity/NetworkMonitor.java
index b9e901b5c5e3..ec4a47930fad 100644
--- a/packages/NetworkStack/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/packages/NetworkStack/src/com/android/server/connectivity/NetworkMonitor.java
@@ -39,9 +39,7 @@ import android.content.BroadcastReceiver;
 import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
-import android.net.CaptivePortal;
 import android.net.ConnectivityManager;
-import android.net.ICaptivePortal;
 import android.net.INetworkMonitor;
 import android.net.INetworkMonitorCallbacks;
 import android.net.LinkProperties;
@@ -460,6 +458,13 @@ public class NetworkMonitor extends StateMachine {
         sendMessage(CMD_LAUNCH_CAPTIVE_PORTAL_APP);
     }
 
+    /**
+     * Notify that the captive portal app was closed with the provided response code.
+     */
+    public void notifyCaptivePortalAppFinished(int response) {
+        sendMessage(CMD_CAPTIVE_PORTAL_APP_FINISHED, response);
+    }
+
     @Override
     protected void log(String s) {
         if (DBG) Log.d(TAG + "/" + mNetwork.toString(), s);
@@ -671,29 +676,8 @@ public class NetworkMonitor extends StateMachine {
                 case CMD_LAUNCH_CAPTIVE_PORTAL_APP:
                     final Bundle appExtras = new Bundle();
                     // OneAddressPerFamilyNetwork is not parcelable across processes.
-                    appExtras.putParcelable(
-                            ConnectivityManager.EXTRA_NETWORK, new Network(mNetwork));
-                    appExtras.putParcelable(ConnectivityManager.EXTRA_CAPTIVE_PORTAL,
-                            new CaptivePortal(new ICaptivePortal.Stub() {
-                                @Override
-                                public void appResponse(int response) {
-                                    if (response == APP_RETURN_WANTED_AS_IS) {
-                                        mContext.enforceCallingPermission(
-                                                PERMISSION_NETWORK_SETTINGS,
-                                                "CaptivePortal");
-                                    }
-                                    sendMessage(CMD_CAPTIVE_PORTAL_APP_FINISHED, response);
-                                }
-
-                                @Override
-                                public void logEvent(int eventId, String packageName)
-                                        throws RemoteException {
-                                    mContext.enforceCallingPermission(
-                                            PERMISSION_NETWORK_SETTINGS,
-                                            "CaptivePortal");
-                                    mCallback.logCaptivePortalLoginEvent(eventId, packageName);
-                                }
-                            }));
+                    final Network network = new Network(mNetwork);
+                    appExtras.putParcelable(ConnectivityManager.EXTRA_NETWORK, network);
                     final CaptivePortalProbeResult probeRes = mLastPortalProbeResult;
                     appExtras.putString(EXTRA_CAPTIVE_PORTAL_URL, probeRes.detectUrl);
                     if (probeRes.probeSpec != null) {
@@ -702,7 +686,7 @@ public class NetworkMonitor extends StateMachine {
                     }
                     appExtras.putString(ConnectivityManager.EXTRA_CAPTIVE_PORTAL_USER_AGENT,
                             mCaptivePortalUserAgent);
-                    mCm.startCaptivePortalApp(appExtras);
+                    mCm.startCaptivePortalApp(network, appExtras);
                     return HANDLED;
                 default:
                     return NOT_HANDLED;
diff --git a/packages/NetworkStack/tests/src/com/android/server/connectivity/NetworkMonitorTest.java b/packages/NetworkStack/tests/src/com/android/server/connectivity/NetworkMonitorTest.java
index b98b0f798fe3..9a16bb77182e 100644
--- a/packages/NetworkStack/tests/src/com/android/server/connectivity/NetworkMonitorTest.java
+++ b/packages/NetworkStack/tests/src/com/android/server/connectivity/NetworkMonitorTest.java
@@ -16,8 +16,7 @@
 
 package com.android.server.connectivity;
 
-import static android.net.ConnectivityManager.ACTION_CAPTIVE_PORTAL_SIGN_IN;
-import static android.net.ConnectivityManager.EXTRA_CAPTIVE_PORTAL;
+import static android.net.CaptivePortal.APP_RETURN_DISMISSED;
 import static android.net.INetworkMonitor.NETWORK_TEST_RESULT_INVALID;
 import static android.net.INetworkMonitor.NETWORK_TEST_RESULT_VALID;
 import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET;
@@ -41,8 +40,6 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import android.content.Context;
-import android.content.Intent;
-import android.net.CaptivePortal;
 import android.net.ConnectivityManager;
 import android.net.INetworkMonitorCallbacks;
 import android.net.InetAddresses;
@@ -54,10 +51,10 @@ import android.net.captiveportal.CaptivePortalProbeResult;
 import android.net.metrics.IpConnectivityLog;
 import android.net.util.SharedLog;
 import android.net.wifi.WifiManager;
+import android.os.Bundle;
 import android.os.ConditionVariable;
 import android.os.Handler;
 import android.os.SystemClock;
-import android.os.UserHandle;
 import android.provider.Settings;
 import android.support.test.filters.SmallTest;
 import android.support.test.runner.AndroidJUnit4;
@@ -487,19 +484,23 @@ public class NetworkMonitorTest {
         // Check that startCaptivePortalApp sends the expected intent.
         nm.launchCaptivePortalApp();
 
-        final ArgumentCaptor<Intent> intentCaptor = ArgumentCaptor.forClass(Intent.class);
-        verify(mContext, timeout(HANDLER_TIMEOUT_MS).times(1))
-                .startActivityAsUser(intentCaptor.capture(), eq(UserHandle.CURRENT));
-        final Intent intent = intentCaptor.getValue();
-        assertEquals(ACTION_CAPTIVE_PORTAL_SIGN_IN, intent.getAction());
-        final Network network = intent.getParcelableExtra(ConnectivityManager.EXTRA_NETWORK);
-        assertEquals(TEST_NETID, network.netId);
+        final ArgumentCaptor<Bundle> bundleCaptor = ArgumentCaptor.forClass(Bundle.class);
+        final ArgumentCaptor<Network> networkCaptor = ArgumentCaptor.forClass(Network.class);
+        verify(mCm, timeout(HANDLER_TIMEOUT_MS).times(1))
+                .startCaptivePortalApp(networkCaptor.capture(), bundleCaptor.capture());
+        final Bundle bundle = bundleCaptor.getValue();
+        final Network bundleNetwork = bundle.getParcelable(ConnectivityManager.EXTRA_NETWORK);
+        assertEquals(TEST_NETID, bundleNetwork.netId);
+        // network is passed both in bundle and as parameter, as the bundle is opaque to the
+        // framework and only intended for the captive portal app, but the framework needs
+        // the network to identify the right NetworkMonitor.
+        assertEquals(TEST_NETID, networkCaptor.getValue().netId);
 
         // Have the app report that the captive portal is dismissed, and check that we revalidate.
         setStatus(mHttpsConnection, 204);
         setStatus(mHttpConnection, 204);
-        final CaptivePortal captivePortal = intent.getParcelableExtra(EXTRA_CAPTIVE_PORTAL);
-        captivePortal.reportCaptivePortalDismissed();
+
+        nm.notifyCaptivePortalAppFinished(APP_RETURN_DISMISSED);
         verify(mCallbacks, timeout(HANDLER_TIMEOUT_MS).times(1))
                 .notifyNetworkTested(NETWORK_TEST_RESULT_VALID, null);
     }
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 9b4cd230427f..eec93808421a 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -57,8 +57,10 @@ import android.content.Intent;
 import android.content.IntentFilter;
 import android.content.res.Configuration;
 import android.database.ContentObserver;
+import android.net.CaptivePortal;
 import android.net.ConnectionInfo;
 import android.net.ConnectivityManager;
+import android.net.ICaptivePortal;
 import android.net.IConnectivityManager;
 import android.net.IIpConnectivityMetrics;
 import android.net.INetd;
@@ -2689,11 +2691,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     EVENT_PROVISIONING_NOTIFICATION, PROVISIONING_NOTIFICATION_HIDE,
                     mNai.network.netId));
         }
-
-        @Override
-        public void logCaptivePortalLoginEvent(int eventId, String packageName) {
-            new MetricsLogger().action(eventId, packageName);
-        }
     }
 
     private boolean networkRequiresValidation(NetworkAgentInfo nai) {
@@ -3239,22 +3236,63 @@ public class ConnectivityService extends IConnectivityManager.Stub
     /**
      * NetworkStack endpoint to start the captive portal app. The NetworkStack needs to use this
      * endpoint as it does not have INTERACT_ACROSS_USERS_FULL itself.
+     * @param network Network on which the captive portal was detected.
      * @param appExtras Bundle to use as intent extras for the captive portal application.
      *                  Must be treated as opaque to avoid preventing the captive portal app to
      *                  update its arguments.
      */
     @Override
-    public void startCaptivePortalAppInternal(Bundle appExtras) {
+    public void startCaptivePortalAppInternal(Network network, Bundle appExtras) {
         mContext.checkCallingOrSelfPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK);
 
         final Intent appIntent = new Intent(ConnectivityManager.ACTION_CAPTIVE_PORTAL_SIGN_IN);
         appIntent.putExtras(appExtras);
+        appIntent.putExtra(ConnectivityManager.EXTRA_CAPTIVE_PORTAL,
+                new CaptivePortal(new CaptivePortalImpl(network).asBinder()));
         appIntent.setFlags(Intent.FLAG_ACTIVITY_BROUGHT_TO_FRONT | Intent.FLAG_ACTIVITY_NEW_TASK);
 
         Binder.withCleanCallingIdentity(() ->
                 mContext.startActivityAsUser(appIntent, UserHandle.CURRENT));
     }
 
+    private class CaptivePortalImpl extends ICaptivePortal.Stub {
+        private final Network mNetwork;
+
+        private CaptivePortalImpl(Network network) {
+            mNetwork = network;
+        }
+
+        @Override
+        public void appResponse(final int response) throws RemoteException {
+            if (response == CaptivePortal.APP_RETURN_WANTED_AS_IS) {
+                enforceSettingsPermission();
+            }
+
+            // getNetworkAgentInfoForNetwork is thread-safe
+            final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(mNetwork);
+            if (nai == null) return;
+
+            // nai.networkMonitor() is thread-safe
+            final INetworkMonitor nm = nai.networkMonitor();
+            if (nm == null) return;
+
+            final long token = Binder.clearCallingIdentity();
+            try {
+                nm.notifyCaptivePortalAppFinished(response);
+            } finally {
+                // Not using Binder.withCleanCallingIdentity() to keep the checked RemoteException
+                Binder.restoreCallingIdentity(token);
+            }
+        }
+
+        @Override
+        public void logEvent(int eventId, String packageName) {
+            enforceSettingsPermission();
+
+            new MetricsLogger().action(eventId, packageName);
+        }
+    }
+
     public boolean avoidBadWifi() {
         return mMultinetworkPolicyTracker.getAvoidBadWifi();
     }