Merge "Revert "Update VPN capabilities when its underlying network set is null."" into pi-dev

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

@@ -867,8 +867,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
         mPermissionMonitor = new PermissionMonitor(mContext, mNetd);
 
-        // Set up the listener for user state for creating user VPNs.
+        //set up the listener for user state for creating user VPNs
-        // Should run on mHandler to avoid any races.
         IntentFilter intentFilter = new IntentFilter();
         intentFilter.addAction(Intent.ACTION_USER_STARTED);
         intentFilter.addAction(Intent.ACTION_USER_STOPPED);
@@ -876,11 +875,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         intentFilter.addAction(Intent.ACTION_USER_REMOVED);
         intentFilter.addAction(Intent.ACTION_USER_UNLOCKED);
         mContext.registerReceiverAsUser(
-                mUserIntentReceiver,
+                mUserIntentReceiver, UserHandle.ALL, intentFilter, null, null);
-                UserHandle.ALL,
-                intentFilter,
-                null /* broadcastPermission */,
-                mHandler);
         mContext.registerReceiverAsUser(mUserPresentReceiver, UserHandle.SYSTEM,
                 new IntentFilter(Intent.ACTION_USER_PRESENT), null, null);
 
@@ -3820,27 +3815,17 @@ public class ConnectivityService extends IConnectivityManager.Stub
      * handler thread through their agent, this is asynchronous. When the capabilities objects
      * are computed they will be up-to-date as they are computed synchronously from here and
      * this is running on the ConnectivityService thread.
+     * TODO : Fix this and call updateCapabilities inline to remove out-of-order events.
      */
     private void updateAllVpnsCapabilities() {
-        Network defaultNetwork = getNetwork(getDefaultNetwork());
         synchronized (mVpns) {
             for (int i = 0; i < mVpns.size(); i++) {
                 final Vpn vpn = mVpns.valueAt(i);
-                NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork);
+                vpn.updateCapabilities();
-                updateVpnCapabilities(vpn, nc);
             }
         }
     }
 
-    private void updateVpnCapabilities(Vpn vpn, @Nullable NetworkCapabilities nc) {
-        ensureRunningOnConnectivityServiceThread();
-        NetworkAgentInfo vpnNai = getNetworkAgentInfoForNetId(vpn.getNetId());
-        if (vpnNai == null || nc == null) {
-            return;
-        }
-        updateCapabilities(vpnNai.getCurrentScore(), vpnNai, nc);
-    }
-
     @Override
     public boolean updateLockdownVpn() {
         if (Binder.getCallingUid() != Process.SYSTEM_UID) {
@@ -4147,27 +4132,21 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     private void onUserAdded(int userId) {
-        Network defaultNetwork = getNetwork(getDefaultNetwork());
         synchronized (mVpns) {
             final int vpnsSize = mVpns.size();
             for (int i = 0; i < vpnsSize; i++) {
                 Vpn vpn = mVpns.valueAt(i);
                 vpn.onUserAdded(userId);
-                NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork);
-                updateVpnCapabilities(vpn, nc);
             }
         }
     }
 
     private void onUserRemoved(int userId) {
-        Network defaultNetwork = getNetwork(getDefaultNetwork());
         synchronized (mVpns) {
             final int vpnsSize = mVpns.size();
             for (int i = 0; i < vpnsSize; i++) {
                 Vpn vpn = mVpns.valueAt(i);
                 vpn.onUserRemoved(userId);
-                NetworkCapabilities nc = vpn.updateCapabilities(defaultNetwork);
-                updateVpnCapabilities(vpn, nc);
             }
         }
     }
@@ -4186,7 +4165,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
     private BroadcastReceiver mUserIntentReceiver = new BroadcastReceiver() {
         @Override
         public void onReceive(Context context, Intent intent) {
-            ensureRunningOnConnectivityServiceThread();
             final String action = intent.getAction();
             final int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, UserHandle.USER_NULL);
             if (userId == UserHandle.USER_NULL) return;
@@ -4672,19 +4650,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return getNetworkForRequest(mDefaultRequest.requestId);
     }
 
-    @Nullable
-    private Network getNetwork(@Nullable NetworkAgentInfo nai) {
-        return nai != null ? nai.network : null;
-    }
-
-    private void ensureRunningOnConnectivityServiceThread() {
-        if (mHandler.getLooper().getThread() != Thread.currentThread()) {
-            throw new IllegalStateException(
-                    "Not running on ConnectivityService thread: "
-                            + Thread.currentThread().getName());
-        }
-    }
-
     private boolean isDefaultNetwork(NetworkAgentInfo nai) {
         return nai == getDefaultNetwork();
     }
@@ -5232,8 +5197,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
         updateTcpBufferSizes(newNetwork);
         mDnsManager.setDefaultDnsSystemProperties(newNetwork.linkProperties.getDnsServers());
         notifyIfacesChangedForNetworkStats();
-        // Fix up the NetworkCapabilities of any VPNs that don't specify underlying networks.
-        updateAllVpnsCapabilities();
     }
 
     private void processListenRequests(NetworkAgentInfo nai, boolean capabilitiesChanged) {
@@ -5667,10 +5630,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
             // doing.
             updateSignalStrengthThresholds(networkAgent, "CONNECT", null);
 
-            if (networkAgent.isVPN()) {
-                updateAllVpnsCapabilities();
-            }
-
             // Consider network even though it is not yet validated.
             final long now = SystemClock.elapsedRealtime();
             rematchNetworkAndRequests(networkAgent, ReapUnvalidatedNetworks.REAP, now);
@@ -5864,11 +5823,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
             success = mVpns.get(user).setUnderlyingNetworks(networks);
         }
         if (success) {
-            mHandler.post(() -> {
+            mHandler.post(() -> notifyIfacesChangedForNetworkStats());
-                // Update VPN's capabilities based on updated underlying network set.
-                updateAllVpnsCapabilities();
-                notifyIfacesChangedForNetworkStats();
-            });
         }
         return success;
     }

services/core/java/com/android/server/connectivity/Vpn.java

@@ -273,7 +273,7 @@ public class Vpn {
         mNetworkCapabilities = new NetworkCapabilities();
         mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN);
         mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN);
-        updateCapabilities(null /* defaultNetwork */);
+        updateCapabilities();
 
         loadAlwaysOnPackage();
     }
@@ -300,39 +300,18 @@ public class Vpn {
         updateAlwaysOnNotification(detailedState);
     }
 
-    /**
+    public void updateCapabilities() {
-     * Updates {@link #mNetworkCapabilities} based on current underlying networks and returns a
+        final Network[] underlyingNetworks = (mConfig != null) ? mConfig.underlyingNetworks : null;
-     * defensive copy.
+        updateCapabilities(mContext.getSystemService(ConnectivityManager.class), underlyingNetworks,
-     *
-     * <p>Does not propagate updated capabilities to apps.
-     *
-     * @param defaultNetwork underlying network for VPNs following platform's default
-     */
-    public synchronized NetworkCapabilities updateCapabilities(
-            @Nullable Network defaultNetwork) {
-        if (mConfig == null) {
-            // VPN is not running.
-            return null;
-        }
-
-        Network[] underlyingNetworks = mConfig.underlyingNetworks;
-        if (underlyingNetworks == null && defaultNetwork != null) {
-            // null underlying networks means to track the default.
-            underlyingNetworks = new Network[] { defaultNetwork };
-        }
-
-        applyUnderlyingCapabilities(
-                mContext.getSystemService(ConnectivityManager.class),
-                underlyingNetworks,
                 mNetworkCapabilities);
 
-        return new NetworkCapabilities(mNetworkCapabilities);
+        if (mNetworkAgent != null) {
+            mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities);
+        }
     }
 
     @VisibleForTesting
-    public static void applyUnderlyingCapabilities(
+    public static void updateCapabilities(ConnectivityManager cm, Network[] underlyingNetworks,
-            ConnectivityManager cm,
-            Network[] underlyingNetworks,
             NetworkCapabilities caps) {
         int[] transportTypes = new int[] { NetworkCapabilities.TRANSPORT_VPN };
         int downKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED;
@@ -344,7 +323,6 @@ public class Vpn {
         boolean hadUnderlyingNetworks = false;
         if (null != underlyingNetworks) {
             for (Network underlying : underlyingNetworks) {
-                // TODO(b/124469351): Get capabilities directly from ConnectivityService instead.
                 final NetworkCapabilities underlyingCaps = cm.getNetworkCapabilities(underlying);
                 if (underlyingCaps == null) continue;
                 hadUnderlyingNetworks = true;
@@ -1015,8 +993,9 @@ public class Vpn {
     }
 
     /**
-     * Establish a VPN network and return the file descriptor of the VPN interface. This methods
+     * Establish a VPN network and return the file descriptor of the VPN
-     * returns {@code null} if the application is revoked or not prepared.
+     * interface. This methods returns {@code null} if the application is
+     * revoked or not prepared.
      *
      * @param config The parameters to configure the network.
      * @return The file descriptor of the VPN interface.
@@ -1263,11 +1242,6 @@ public class Vpn {
         return ranges;
     }
 
-    /**
-     * Updates UID ranges for this VPN and also updates its internal capabilities.
-     *
-     * <p>Should be called on primary ConnectivityService thread.
-     */
     public void onUserAdded(int userHandle) {
         // If the user is restricted tie them to the parent user's VPN
         UserInfo user = UserManager.get(mContext).getUserInfo(userHandle);
@@ -1278,9 +1252,8 @@ public class Vpn {
                     try {
                         addUserToRanges(existingRanges, userHandle, mConfig.allowedApplications,
                                 mConfig.disallowedApplications);
-                        // ConnectivityService will call {@link #updateCapabilities} and apply
-                        // those for VPN network.
                         mNetworkCapabilities.setUids(existingRanges);
+                        updateCapabilities();
                     } catch (Exception e) {
                         Log.wtf(TAG, "Failed to add restricted user to owner", e);
                     }
@@ -1290,11 +1263,6 @@ public class Vpn {
         }
     }
 
-    /**
-     * Updates UID ranges for this VPN and also updates its capabilities.
-     *
-     * <p>Should be called on primary ConnectivityService thread.
-     */
     public void onUserRemoved(int userHandle) {
         // clean up if restricted
         UserInfo user = UserManager.get(mContext).getUserInfo(userHandle);
@@ -1306,9 +1274,8 @@ public class Vpn {
                         final List<UidRange> removedRanges =
                             uidRangesForUser(userHandle, existingRanges);
                         existingRanges.removeAll(removedRanges);
-                        // ConnectivityService will call {@link #updateCapabilities} and
-                        // apply those for VPN network.
                         mNetworkCapabilities.setUids(existingRanges);
+                        updateCapabilities();
                     } catch (Exception e) {
                         Log.wtf(TAG, "Failed to remove restricted user to owner", e);
                     }
@@ -1516,12 +1483,6 @@ public class Vpn {
         return success;
     }
 
-    /**
-     * Updates underlying network set.
-     *
-     * <p>Note: Does not updates capabilities. Call {@link #updateCapabilities} from
-     * ConnectivityService thread to get updated capabilities.
-     */
     public synchronized boolean setUnderlyingNetworks(Network[] networks) {
         if (!isCallerEstablishedOwnerLocked()) {
             return false;
@@ -1538,6 +1499,7 @@ public class Vpn {
                 }
             }
         }
+        updateCapabilities();
         return true;
     }
 

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

@@ -20,7 +20,6 @@ import static android.net.ConnectivityManager.CONNECTIVITY_ACTION;
 import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OFF;
 import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC;
 import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_PROVIDER_HOSTNAME;
-import static android.net.ConnectivityManager.NETID_UNSET;
 import static android.net.ConnectivityManager.TYPE_ETHERNET;
 import static android.net.ConnectivityManager.TYPE_MOBILE;
 import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA;
@@ -776,14 +775,11 @@ public class ConnectivityServiceTest {
 
         public void setUids(Set<UidRange> uids) {
             mNetworkCapabilities.setUids(uids);
-            updateCapabilities(null /* defaultNetwork */);
+            updateCapabilities();
         }
 
         @Override
         public int getNetId() {
-            if (mMockNetworkAgent == null) {
-                return NETID_UNSET;
-            }
             return mMockNetworkAgent.getNetwork().netId;
         }
 
@@ -804,13 +800,12 @@ public class ConnectivityServiceTest {
         }
 
         @Override
-        public NetworkCapabilities updateCapabilities(Network defaultNetwork) {
+        public void updateCapabilities() {
-            if (!mConnected) return null;
+            if (!mConnected) return;
-            super.updateCapabilities(defaultNetwork);
+            super.updateCapabilities();
-            // Because super.updateCapabilities will update the capabilities of the agent but
+            // Because super.updateCapabilities will update the capabilities of the agent but not
-            // not the mock agent, the mock agent needs to know about them.
+            // the mock agent, the mock agent needs to know about them.
             copyCapabilitiesToNetworkAgent();
-            return new NetworkCapabilities(mNetworkCapabilities);
         }
 
         private void copyCapabilitiesToNetworkAgent() {
@@ -4223,7 +4218,6 @@ public class ConnectivityServiceTest {
         mMockVpn.setUids(ranges);
         vpnNetworkAgent.connect(false);
         mMockVpn.connect();
-        mMockVpn.setUnderlyingNetworks(new Network[0]);
 
         genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent);
         genericNotVpnNetworkCallback.assertNoCallback();
@@ -4256,7 +4250,6 @@ public class ConnectivityServiceTest {
 
         ranges.add(new UidRange(uid, uid));
         mMockVpn.setUids(ranges);
-        vpnNetworkAgent.setUids(ranges);
 
         genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent);
         genericNotVpnNetworkCallback.assertNoCallback();
@@ -4290,11 +4283,12 @@ public class ConnectivityServiceTest {
     }
 
     @Test
-    public void testVpnWithoutInternet() {
+    public void testVpnWithAndWithoutInternet() {
         final int uid = Process.myUid();
 
         final TestNetworkCallback defaultCallback = new TestNetworkCallback();
         mCm.registerDefaultNetworkCallback(defaultCallback);
+        defaultCallback.assertNoCallback();
 
         mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
         mWiFiNetworkAgent.connect(true);
@@ -4316,30 +4310,11 @@ public class ConnectivityServiceTest {
         vpnNetworkAgent.disconnect();
         defaultCallback.assertNoCallback();
 
-        mCm.unregisterNetworkCallback(defaultCallback);
+        vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN);
-    }
-
-    @Test
-    public void testVpnWithInternet() {
-        final int uid = Process.myUid();
-
-        final TestNetworkCallback defaultCallback = new TestNetworkCallback();
-        mCm.registerDefaultNetworkCallback(defaultCallback);
-
-        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
-        mWiFiNetworkAgent.connect(true);
-
-        defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent);
-        assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork());
-
-        MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN);
-        final ArraySet<UidRange> ranges = new ArraySet<>();
-        ranges.add(new UidRange(uid, uid));
         mMockVpn.setNetworkAgent(vpnNetworkAgent);
         mMockVpn.setUids(ranges);
         vpnNetworkAgent.connect(true /* validated */, true /* hasInternet */);
         mMockVpn.connect();
-
         defaultCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent);
         assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork());
 
@@ -4347,6 +4322,14 @@ public class ConnectivityServiceTest {
         defaultCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent);
         defaultCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent);
 
+        vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN);
+        ranges.clear();
+        mMockVpn.setNetworkAgent(vpnNetworkAgent);
+        mMockVpn.setUids(ranges);
+        vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */);
+        mMockVpn.connect();
+        defaultCallback.assertNoCallback();
+
         mCm.unregisterNetworkCallback(defaultCallback);
     }
 
@@ -4447,68 +4430,4 @@ public class ConnectivityServiceTest {
 
         mMockVpn.disconnect();
     }
-
-    @Test
-    public void testNullUnderlyingNetworks() {
-        final int uid = Process.myUid();
-
-        final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback();
-        final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder()
-                .removeCapability(NET_CAPABILITY_NOT_VPN)
-                .addTransportType(TRANSPORT_VPN)
-                .build();
-        NetworkCapabilities nc;
-        mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback);
-        vpnNetworkCallback.assertNoCallback();
-
-        final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN);
-        final ArraySet<UidRange> ranges = new ArraySet<>();
-        ranges.add(new UidRange(uid, uid));
-        mMockVpn.setNetworkAgent(vpnNetworkAgent);
-        mMockVpn.connect();
-        mMockVpn.setUids(ranges);
-        vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */);
-
-        vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent);
-        nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork());
-        assertTrue(nc.hasTransport(TRANSPORT_VPN));
-        assertFalse(nc.hasTransport(TRANSPORT_CELLULAR));
-        assertFalse(nc.hasTransport(TRANSPORT_WIFI));
-        // By default, VPN is set to track default network (i.e. its underlying networks is null).
-        // In case of no default network, VPN is considered metered.
-        assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED));
-
-        // Connect to Cell; Cell is the default network.
-        mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
-        mCellNetworkAgent.connect(true);
-
-        vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN)
-                && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI)
-                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED),
-                vpnNetworkAgent);
-
-        // Connect to WiFi; WiFi is the new default.
-        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
-        mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED);
-        mWiFiNetworkAgent.connect(true);
-
-        vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN)
-                && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI)
-                && caps.hasCapability(NET_CAPABILITY_NOT_METERED),
-                vpnNetworkAgent);
-
-        // Disconnect Cell. The default network did not change, so there shouldn't be any changes in
-        // the capabilities.
-        mCellNetworkAgent.disconnect();
-
-        // Disconnect wifi too. Now we have no default network.
-        mWiFiNetworkAgent.disconnect();
-
-        vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN)
-                && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI)
-                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED),
-                vpnNetworkAgent);
-
-        mMockVpn.disconnect();
-    }
 }

tests/net/java/com/android/server/connectivity/VpnTest.java

@@ -457,8 +457,7 @@ public class VpnTest {
 
         final NetworkCapabilities caps = new NetworkCapabilities();
 
-        Vpn.applyUnderlyingCapabilities(
+        Vpn.updateCapabilities(mConnectivityManager, new Network[] { }, caps);
-                mConnectivityManager, new Network[] {}, caps);
         assertTrue(caps.hasTransport(TRANSPORT_VPN));
         assertFalse(caps.hasTransport(TRANSPORT_CELLULAR));
         assertFalse(caps.hasTransport(TRANSPORT_WIFI));
@@ -468,8 +467,7 @@ public class VpnTest {
         assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));
         assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));
 
-        Vpn.applyUnderlyingCapabilities(
+        Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile }, caps);
-                mConnectivityManager, new Network[] {mobile}, caps);
         assertTrue(caps.hasTransport(TRANSPORT_VPN));
         assertTrue(caps.hasTransport(TRANSPORT_CELLULAR));
         assertFalse(caps.hasTransport(TRANSPORT_WIFI));
@@ -479,8 +477,7 @@ public class VpnTest {
         assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));
         assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));
 
-        Vpn.applyUnderlyingCapabilities(
+        Vpn.updateCapabilities(mConnectivityManager, new Network[] { wifi }, caps);
-                mConnectivityManager, new Network[] {wifi}, caps);
         assertTrue(caps.hasTransport(TRANSPORT_VPN));
         assertFalse(caps.hasTransport(TRANSPORT_CELLULAR));
         assertTrue(caps.hasTransport(TRANSPORT_WIFI));
@@ -490,8 +487,7 @@ public class VpnTest {
         assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));
         assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));
 
-        Vpn.applyUnderlyingCapabilities(
+        Vpn.updateCapabilities(mConnectivityManager, new Network[] { mobile, wifi }, caps);
-                mConnectivityManager, new Network[] {mobile, wifi}, caps);
         assertTrue(caps.hasTransport(TRANSPORT_VPN));
         assertTrue(caps.hasTransport(TRANSPORT_CELLULAR));
         assertTrue(caps.hasTransport(TRANSPORT_WIFI));