Merge "Unbreak extraInfo values returned to apps."

services/core/java/com/android/server/ConnectivityService.java

@@ -1514,11 +1514,11 @@ public class ConnectivityService extends IConnectivityManager.Stub
         // but only exists if an app asks about them or requests them. Ensure the requesting app
         // gets the type it asks for.
         filtered.setType(type);
-        final DetailedState state = isNetworkWithCapabilitiesBlocked(nc, uid, ignoreBlocked)
+        if (isNetworkWithCapabilitiesBlocked(nc, uid, ignoreBlocked)) {
-                ? DetailedState.BLOCKED
+            filtered.setDetailedState(DetailedState.BLOCKED, null /* reason */,
-                : filtered.getDetailedState();
+                    null /* extraInfo */);
-        filtered.setDetailedState(getLegacyLockdownState(state),
+        }
-                "" /* reason */, null /* extraInfo */);
+        filterForLegacyLockdown(filtered);
         return filtered;
     }
 
@@ -1594,8 +1594,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
         final DetailedState state = isNetworkWithCapabilitiesBlocked(nc, uid, false)
                 ? DetailedState.BLOCKED
                 : DetailedState.DISCONNECTED;
-        info.setDetailedState(getLegacyLockdownState(state),
+        info.setDetailedState(state, null /* reason */, null /* extraInfo */);
-                "" /* reason */, null /* extraInfo */);
+        filterForLegacyLockdown(info);
         return info;
     }
 
@@ -2389,9 +2389,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         mContext.enforceCallingOrSelfPermission(KeepaliveTracker.PERMISSION, "ConnectivityService");
     }
 
-    // Public because it's used by mLockdownTracker.
+    private void sendConnectedBroadcast(NetworkInfo info) {
-    public void sendConnectedBroadcast(NetworkInfo info) {
-        PermissionUtils.enforceNetworkStackPermission(mContext);
         sendGeneralBroadcast(info, CONNECTIVITY_ACTION);
     }
 
@@ -5036,8 +5034,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
         // The legacy lockdown VPN always uses the default network.
         // If the VPN's underlying network is no longer the current default network, it means that
         // the default network has just switched, and the VPN is about to disconnect.
-        // Report that the VPN is not connected, so when the state of NetworkInfo objects
+        // Report that the VPN is not connected, so the state of NetworkInfo objects overwritten
-        // overwritten by getLegacyLockdownState will be set to CONNECTING and not CONNECTED.
+        // by filterForLegacyLockdown will be set to CONNECTING and not CONNECTED.
         final NetworkAgentInfo defaultNetwork = getDefaultNetwork();
         if (defaultNetwork == null || !defaultNetwork.network.equals(underlying[0])) {
             return null;
@@ -5046,6 +5044,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return nai;
     };
 
+    // TODO: move all callers to filterForLegacyLockdown and delete this method.
+    // This likely requires making sendLegacyNetworkBroadcast take a NetworkInfo object instead of
+    // just a DetailedState object.
     private DetailedState getLegacyLockdownState(DetailedState origState) {
         if (origState != DetailedState.CONNECTED) {
             return origState;
@@ -5055,6 +5056,23 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 : DetailedState.CONNECTED;
     }
 
+    private void filterForLegacyLockdown(NetworkInfo ni) {
+        if (!mLockdownEnabled || !ni.isConnected()) return;
+        // The legacy lockdown VPN replaces the state of every network in CONNECTED state with the
+        // state of its VPN. This is to ensure that when an underlying network connects, apps will
+        // not see a CONNECTIVITY_ACTION broadcast for a network in state CONNECTED until the VPN
+        // comes up, at which point there is a new CONNECTIVITY_ACTION broadcast for the underlying
+        // network, this time with a state of CONNECTED.
+        //
+        // Now that the legacy lockdown code lives in ConnectivityService, and no longer has access
+        // to the internal state of the Vpn object, always replace the state with CONNECTING. This
+        // is not too far off the truth, since an always-on VPN, when not connected, is always
+        // trying to reconnect.
+        if (getLegacyLockdownNai() == null) {
+            ni.setDetailedState(DetailedState.CONNECTING, "", null);
+        }
+    }
+
     @Override
     public void setProvisioningNotificationVisible(boolean visible, int networkType,
             String action) {
@@ -7938,6 +7956,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         // and is still connected.
         NetworkInfo info = new NetworkInfo(nai.networkInfo);
         info.setType(type);
+        filterForLegacyLockdown(info);
         if (state != DetailedState.DISCONNECTED) {
             info.setDetailedState(state, null, info.getExtraInfo());
             sendConnectedBroadcast(info);

tests/net/java/com/android/server/ConnectivityServiceTest.java

@@ -1740,11 +1740,29 @@ public class ConnectivityServiceTest {
         return expected;
     }
 
+    private boolean extraInfoInBroadcastHasExpectedNullness(NetworkInfo ni) {
+        final DetailedState state = ni.getDetailedState();
+        if (state == DetailedState.CONNECTED && ni.getExtraInfo() == null) return false;
+        // Expect a null extraInfo if the network is CONNECTING, because a CONNECTIVITY_ACTION
+        // broadcast with a state of CONNECTING only happens due to legacy VPN lockdown, which also
+        // nulls out extraInfo.
+        if (state == DetailedState.CONNECTING && ni.getExtraInfo() != null) return false;
+        // Can't make any assertions about DISCONNECTED broadcasts. When a network actually
+        // disconnects, disconnectAndDestroyNetwork sets its state to DISCONNECTED and its extraInfo
+        // to null. But if the DISCONNECTED broadcast is just simulated by LegacyTypeTracker due to
+        // a network switch, extraInfo will likely be populated.
+        // This is likely a bug in CS, but likely not one we can fix without impacting apps.
+        return true;
+    }
+
     private ExpectedBroadcast expectConnectivityAction(int type, NetworkInfo.DetailedState state) {
-        return registerConnectivityBroadcastThat(1, intent ->
+        return registerConnectivityBroadcastThat(1, intent -> {
-                type == intent.getIntExtra(EXTRA_NETWORK_TYPE, -1) && state.equals(
+            final int actualType = intent.getIntExtra(EXTRA_NETWORK_TYPE, -1);
-                        ((NetworkInfo) intent.getParcelableExtra(EXTRA_NETWORK_INFO))
+            final NetworkInfo ni = intent.getParcelableExtra(EXTRA_NETWORK_INFO);
-                                .getDetailedState()));
+            return type == actualType
+                    && state == ni.getDetailedState()
+                    && extraInfoInBroadcastHasExpectedNullness(ni);
+        });
     }
 
     @Test
@@ -7182,12 +7200,14 @@ public class ConnectivityServiceTest {
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
 
         setUidRulesChanged(RULE_REJECT_ALL);
         cellNetworkCallback.expectBlockedStatusCallback(true, mCellNetworkAgent);
         assertNull(mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
+        assertExtraInfoFromCmBlocked(mCellNetworkAgent);
 
         // ConnectivityService should cache it not to invoke the callback again.
         setUidRulesChanged(RULE_REJECT_METERED);
@@ -7198,12 +7218,14 @@ public class ConnectivityServiceTest {
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
 
         setUidRulesChanged(RULE_REJECT_METERED);
         cellNetworkCallback.expectBlockedStatusCallback(true, mCellNetworkAgent);
         assertNull(mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
+        assertExtraInfoFromCmBlocked(mCellNetworkAgent);
 
         // Restrict the network based on UID rule and NOT_METERED capability change.
         mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED);
@@ -7212,6 +7234,7 @@ public class ConnectivityServiceTest {
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
 
         mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_METERED);
         cellNetworkCallback.expectCapabilitiesWithout(NET_CAPABILITY_NOT_METERED,
@@ -7220,12 +7243,14 @@ public class ConnectivityServiceTest {
         assertNull(mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
+        assertExtraInfoFromCmBlocked(mCellNetworkAgent);
 
         setUidRulesChanged(RULE_ALLOW_METERED);
         cellNetworkCallback.expectBlockedStatusCallback(false, mCellNetworkAgent);
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
 
         setUidRulesChanged(RULE_NONE);
         cellNetworkCallback.assertNoCallback();
@@ -7236,6 +7261,7 @@ public class ConnectivityServiceTest {
         assertNull(mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
+        assertExtraInfoFromCmBlocked(mCellNetworkAgent);
         setRestrictBackgroundChanged(true);
         cellNetworkCallback.assertNoCallback();
 
@@ -7243,12 +7269,14 @@ public class ConnectivityServiceTest {
         cellNetworkCallback.expectBlockedStatusCallback(false, mCellNetworkAgent);
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
 
         setRestrictBackgroundChanged(false);
         cellNetworkCallback.assertNoCallback();
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
         assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
 
         mCm.unregisterNetworkCallback(cellNetworkCallback);
     }
@@ -7307,6 +7335,15 @@ public class ConnectivityServiceTest {
         assertNotNull(ni);
         assertEquals(type, ni.getType());
         assertEquals(ConnectivityManager.getNetworkTypeName(type), state, ni.getDetailedState());
+        if (state == DetailedState.CONNECTED || state == DetailedState.SUSPENDED) {
+            assertNotNull(ni.getExtraInfo());
+        } else {
+            // Technically speaking, a network that's in CONNECTING state will generally have a
+            // non-null extraInfo. This doesn't actually happen in this test because it never calls
+            // a legacy API while a network is connecting. When a network is in CONNECTING state
+            // because of legacy lockdown VPN, its extraInfo is always null.
+            assertNull(ni.getExtraInfo());
+        }
     }
 
     private void assertActiveNetworkInfo(int type, DetailedState state) {
@@ -7316,6 +7353,26 @@ public class ConnectivityServiceTest {
         checkNetworkInfo(mCm.getNetworkInfo(type), type, state);
     }
 
+    private void assertExtraInfoFromCm(TestNetworkAgentWrapper network, boolean present) {
+        final NetworkInfo niForNetwork = mCm.getNetworkInfo(network.getNetwork());
+        final NetworkInfo niForType = mCm.getNetworkInfo(network.getLegacyType());
+        if (present) {
+            assertEquals(network.getExtraInfo(), niForNetwork.getExtraInfo());
+            assertEquals(network.getExtraInfo(), niForType.getExtraInfo());
+        } else {
+            assertNull(niForNetwork.getExtraInfo());
+            assertNull(niForType.getExtraInfo());
+        }
+    }
+
+    private void assertExtraInfoFromCmBlocked(TestNetworkAgentWrapper network) {
+        assertExtraInfoFromCm(network, false);
+    }
+
+    private void assertExtraInfoFromCmPresent(TestNetworkAgentWrapper network) {
+        assertExtraInfoFromCm(network, true);
+    }
+
     // Checks that each of the |agents| receive a blocked status change callback with the specified
     // |blocked| value, in any order. This is needed because when an event affects multiple
     // networks, ConnectivityService does not guarantee the order in which callbacks are fired.
@@ -7630,6 +7687,7 @@ public class ConnectivityServiceTest {
         assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_WIFI, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_VPN, DetailedState.BLOCKED);
+        assertExtraInfoFromCmBlocked(mCellNetworkAgent);
 
         // TODO: it would be nice if we could simply rely on the production code here, and have
         // LockdownVpnTracker start the VPN, have the VPN code register its NetworkAgent with
@@ -7658,6 +7716,7 @@ public class ConnectivityServiceTest {
         assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED);
         assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mCellNetworkAgent);
         assertTrue(vpnNc.hasTransport(TRANSPORT_VPN));
         assertTrue(vpnNc.hasTransport(TRANSPORT_CELLULAR));
         assertFalse(vpnNc.hasTransport(TRANSPORT_WIFI));
@@ -7700,6 +7759,7 @@ public class ConnectivityServiceTest {
         assertNetworkInfo(TYPE_MOBILE, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_WIFI, DetailedState.BLOCKED);
         assertNetworkInfo(TYPE_VPN, DetailedState.BLOCKED);
+        assertExtraInfoFromCmBlocked(mWiFiNetworkAgent);
 
         // The VPN comes up again on wifi.
         b1 = expectConnectivityAction(TYPE_VPN, DetailedState.CONNECTED);
@@ -7714,6 +7774,7 @@ public class ConnectivityServiceTest {
         assertNetworkInfo(TYPE_MOBILE, DetailedState.DISCONNECTED);
         assertNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mWiFiNetworkAgent);
         vpnNc = mCm.getNetworkCapabilities(mMockVpn.getNetwork());
         assertTrue(vpnNc.hasTransport(TRANSPORT_VPN));
         assertTrue(vpnNc.hasTransport(TRANSPORT_WIFI));
@@ -7730,6 +7791,7 @@ public class ConnectivityServiceTest {
         assertNetworkInfo(TYPE_MOBILE, DetailedState.DISCONNECTED);
         assertNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED);
         assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED);
+        assertExtraInfoFromCmPresent(mWiFiNetworkAgent);
 
         b1 = expectConnectivityAction(TYPE_WIFI, DetailedState.DISCONNECTED);
         mWiFiNetworkAgent.disconnect();