From 99fb6920f69d31088becad6fd5f39c7200c2c6b9 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 19 Nov 2020 18:55:12 +0900 Subject: [PATCH 1/3] Add a test for restricted profile added/removed with VPN up. Bug: 173331190 Test: test-only change Change-Id: Ibbf96a259a73068d110a159d54059720121117cc --- .../server/ConnectivityServiceTest.java | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 6293befb2c..0595ac9032 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -18,6 +18,8 @@ package com.android.server; import static android.Manifest.permission.CHANGE_NETWORK_STATE; import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS; +import static android.content.Intent.ACTION_USER_ADDED; +import static android.content.Intent.ACTION_USER_REMOVED; import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED; import static android.content.pm.PackageManager.GET_PERMISSIONS; import static android.content.pm.PackageManager.MATCH_ANY_USER; @@ -5880,6 +5882,75 @@ public class ConnectivityServiceTest { mMockVpn.disconnect(); } + @Test + public void testVpnRestrictedUsers() throws Exception { + // NETWORK_SETTINGS is necessary to see the UID ranges in NetworkCapabilities. + mServiceContext.setPermission(Manifest.permission.NETWORK_SETTINGS, + PERMISSION_GRANTED); + + final NetworkRequest request = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN) + .build(); + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerNetworkCallback(request, callback); + + // Bring up a VPN + mMockVpn.establishForMyUid(); + callback.expectAvailableThenValidatedCallbacks(mMockVpn); + callback.assertNoCallback(); + + final int uid = Process.myUid(); + NetworkCapabilities nc = mCm.getNetworkCapabilities(mMockVpn.getNetwork()); + assertNotNull("nc=" + nc, nc.getUids()); + assertEquals(nc.getUids(), uidRangesForUid(uid)); + + // Set an underlying network and expect to see the VPN transports change. + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(true); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + callback.expectCapabilitiesThat(mMockVpn, (caps) + -> caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_WIFI)); + callback.expectCapabilitiesThat(mWiFiNetworkAgent, (caps) + -> caps.hasCapability(NET_CAPABILITY_VALIDATED)); + + // Create a fake restricted profile whose parent is our user ID. + final int userId = UserHandle.getUserId(uid); + final int restrictedUserId = userId + 1; + final UserInfo info = new UserInfo(restrictedUserId, "user", UserInfo.FLAG_RESTRICTED); + info.restrictedProfileParentId = userId; + assertTrue(info.isRestricted()); + when(mUserManager.getUserInfo(restrictedUserId)).thenReturn(info); + final Intent addedIntent = new Intent(ACTION_USER_ADDED); + addedIntent.putExtra(Intent.EXTRA_USER_HANDLE, restrictedUserId); + + // Send a USER_ADDED broadcast for it. + // The BroadcastReceiver for this broadcast checks that is being run on the handler thread. + final Handler handler = new Handler(mCsHandlerThread.getLooper()); + handler.post(() -> mServiceContext.sendBroadcast(addedIntent)); + + // Expect that the VPN UID ranges contain both |uid| and the UID range for the newly-added + // restricted user. + callback.expectCapabilitiesThat(mMockVpn, (caps) + -> caps.getUids().size() == 2 + && caps.getUids().contains(new UidRange(uid, uid)) + && caps.getUids().contains(UidRange.createForUser(restrictedUserId)) + && caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_WIFI)); + + // Send a USER_REMOVED broadcast and expect to lose the UID range for the restricted user. + final Intent removedIntent = new Intent(ACTION_USER_REMOVED); + removedIntent.putExtra(Intent.EXTRA_USER_HANDLE, restrictedUserId); + handler.post(() -> mServiceContext.sendBroadcast(removedIntent)); + + // Expect that the VPN gains the UID range for the restricted user. + callback.expectCapabilitiesThat(mMockVpn, (caps) + -> caps.getUids().size() == 1 + && caps.getUids().contains(new UidRange(uid, uid)) + && caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_WIFI)); + } + @Test public void testIsActiveNetworkMeteredOverWifi() throws Exception { // Returns true by default when no network is available. From b7769533c162dc316164d933c917c3856982fe47 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 19 Nov 2020 23:20:55 +0900 Subject: [PATCH 2/3] Make testVpnNetworkActive more deterministic. This test is a bit brittle because it sets the underlying networks while the VPN is undergoing validation by NetworkMonitor. The test does attempt to disable validation, but that's not actually possible - the only thing that's possible is to tell NetworkMonitor to validate immediately without sending any probes. So the underlying network change races with the validation. I'm not sure why the test isn't flaky. It might be because both the network change and the validation result in a capabilities change, and the test expects "a capabilities change" without expressing what change that should be. Make this a bit more predictable by ensuring that the network validates before the underlying networks are set. This is useful because an upcoming CL will change the way underlying network capabilities are propagated. With this test CL, both the old and the new code pass. Bug: 173331190 Test: test-only change Change-Id: I319858228e8d097c0b60a107029f296385f91269 --- .../android/server/ConnectivityServiceTest.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 0595ac9032..f2450038ff 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5470,6 +5470,7 @@ public class ConnectivityServiceTest { final Set ranges = uidRangesForUid(uid); mMockVpn.registerAgent(ranges); + mService.setUnderlyingNetworksForVpn(new Network[0]); // VPN networks do not satisfy the default request and are automatically validated // by NetworkMonitor @@ -5478,19 +5479,12 @@ public class ConnectivityServiceTest { mMockVpn.getAgent().setNetworkValid(false /* isStrictMode */); mMockVpn.connect(false); - mService.setUnderlyingNetworksForVpn(new Network[0]); - genericNetworkCallback.expectAvailableCallbacksUnvalidated(mMockVpn); + genericNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); wifiNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectAvailableCallbacksUnvalidated(mMockVpn); - defaultCallback.expectAvailableCallbacksUnvalidated(mMockVpn); - assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - - genericNetworkCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); - genericNotVpnNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, nc -> null == nc.getUids()); - defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); + vpnNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); + defaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); ranges.clear(); From 6ae66818928f482f8652c66395076b2cc1bee029 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 25 Nov 2020 22:59:08 +0900 Subject: [PATCH 3/3] Test passing an underlying network array with null network in it. Current code treats these nulls as if they weren't there. Bug: 173331190 Test: test-only change Change-Id: Id4632e1b004c09910b4b7613f7233d2c19e2f0ac --- .../server/ConnectivityServiceTest.java | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index f2450038ff..bc99c11942 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -4943,8 +4943,6 @@ public class ConnectivityServiceTest { final Network[] cellAndVpn = new Network[] { mCellNetworkAgent.getNetwork(), mMockVpn.getNetwork()}; - Network[] cellAndWifi = new Network[] { - mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; // A VPN with default (null) underlying networks sets the underlying network's interfaces... expectForceUpdateIfaces(cellAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, @@ -4954,10 +4952,13 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(false); mWiFiNetworkAgent.sendLinkProperties(wifiLp); + final Network[] onlyNull = new Network[]{null}; final Network[] wifiAndVpn = new Network[] { mWiFiNetworkAgent.getNetwork(), mMockVpn.getNetwork()}; - cellAndWifi = new Network[] { + final Network[] cellAndWifi = new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; + final Network[] cellNullAndWifi = new Network[] { + mCellNetworkAgent.getNetwork(), null, mWiFiNetworkAgent.getNetwork()}; waitForIdle(); assertEquals(wifiLp, mService.getActiveLinkProperties()); @@ -4983,6 +4984,13 @@ public class ConnectivityServiceTest { new String[]{MOBILE_IFNAME, WIFI_IFNAME}); reset(mStatsService); + // Null underlying networks are ignored. + mService.setUnderlyingNetworksForVpn(cellNullAndWifi); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{MOBILE_IFNAME, WIFI_IFNAME}); + reset(mStatsService); + // If an underlying network disconnects, that interface should no longer be underlying. // This doesn't actually work because disconnectAndDestroyNetwork only notifies // NetworkStatsService before the underlying network is actually removed. So the underlying @@ -5017,6 +5025,7 @@ public class ConnectivityServiceTest { argThat(vpnInfos -> vpnInfos[0].underlyingIfaces.length == 1 && WIFI_IFNAME.equals(vpnInfos[0].underlyingIfaces[0]))); mEthernetNetworkAgent.disconnect(); + waitForIdle(); reset(mStatsService); // When a VPN declares no underlying networks (i.e., no connectivity), getAllVpnInfo @@ -5029,6 +5038,18 @@ public class ConnectivityServiceTest { waitForIdle(); expectForceUpdateIfaces(wifiAndVpn, null); reset(mStatsService); + + // Specifying only a null underlying network is the same as no networks. + mService.setUnderlyingNetworksForVpn(onlyNull); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, null); + reset(mStatsService); + + // Specifying networks that are all disconnected is the same as specifying no networks. + mService.setUnderlyingNetworksForVpn(onlyCell); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, null); + reset(mStatsService); } @Test