From d7caf838c034acf212b89685297156227fface1f Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 1 Dec 2020 01:08:37 +0900 Subject: [PATCH] Refactor applyUnderlyingCapabilities and its test. This reduces verbose assertions and makes the test more compact. I'm not sure whether it's actually more valuable, since the current code, while more verbose, is probably more straightforward to understand. Also add a test for passing in a null underlying network (i.e., follow default network). This requires a minor refactoring in ConnectivityService because the applyUnderlyingCapabilities does not currently treat null specially. Bug: 173331190 Test: test-only change Change-Id: Ic5a3e16969ea9e1a529706850f148cb0d5fd8e09 --- .../android/server/ConnectivityService.java | 23 ++-- .../server/ConnectivityServiceTest.java | 113 +++++++++--------- 2 files changed, 67 insertions(+), 69 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 6ef1b6bbd5..9b006d050c 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6355,9 +6355,15 @@ public class ConnectivityService extends IConnectivityManager.Stub nai.declaredMetered = !nc.hasCapability(NET_CAPABILITY_NOT_METERED); } + /** Modifies |caps| based on the capabilities of the specified underlying networks. */ @VisibleForTesting - void applyUnderlyingCapabilities(Network[] underlyingNetworks, - NetworkCapabilities caps, boolean declaredMetered) { + void applyUnderlyingCapabilities(@Nullable Network[] underlyingNetworks, + @NonNull NetworkCapabilities caps, boolean declaredMetered) { + final Network defaultNetwork = getNetwork(getDefaultNetwork()); + if (underlyingNetworks == null && defaultNetwork != null) { + // null underlying networks means to track the default. + underlyingNetworks = new Network[] { defaultNetwork }; + } int[] transportTypes = new int[] { NetworkCapabilities.TRANSPORT_VPN }; int downKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED; int upKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED; @@ -6416,17 +6422,6 @@ public class ConnectivityService extends IConnectivityManager.Stub caps.setCapability(NET_CAPABILITY_NOT_SUSPENDED, !suspended); } - /** Propagates to |nc| the capabilities declared by the underlying networks of |nai|. */ - private void mixInUnderlyingCapabilities(NetworkAgentInfo nai, NetworkCapabilities nc) { - Network[] underlyingNetworks = nai.declaredUnderlyingNetworks; - Network defaultNetwork = getNetwork(getDefaultNetwork()); - if (underlyingNetworks == null && defaultNetwork != null) { - // null underlying networks means to track the default. - underlyingNetworks = new Network[] { defaultNetwork }; - } - applyUnderlyingCapabilities(underlyingNetworks, nc, nai.declaredMetered); - } - /** * Augments the NetworkCapabilities passed in by a NetworkAgent with capabilities that are * maintained here that the NetworkAgent is not aware of (e.g., validated, captive portal, @@ -6481,7 +6476,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } if (nai.supportsUnderlyingNetworks()) { - mixInUnderlyingCapabilities(nai, newNc); + applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks, newNc, nai.declaredMetered); } return newNc; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 978c09ef13..878f6fa0f9 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -46,7 +46,6 @@ import static android.net.INetworkMonitor.NETWORK_VALIDATION_PROBE_HTTPS; import static android.net.INetworkMonitor.NETWORK_VALIDATION_PROBE_PRIVDNS; import static android.net.INetworkMonitor.NETWORK_VALIDATION_RESULT_PARTIAL; import static android.net.INetworkMonitor.NETWORK_VALIDATION_RESULT_VALID; -import static android.net.NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED; import static android.net.NetworkCapabilities.NET_CAPABILITY_CAPTIVE_PORTAL; import static android.net.NetworkCapabilities.NET_CAPABILITY_CBS; import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN; @@ -5427,73 +5426,77 @@ public class ConnectivityServiceTest { final Network mobile = mCellNetworkAgent.getNetwork(); final Network wifi = mWiFiNetworkAgent.getNetwork(); - final NetworkCapabilities caps = new NetworkCapabilities(); + final NetworkCapabilities initialCaps = new NetworkCapabilities(); + initialCaps.addCapability(NET_CAPABILITY_INTERNET); + initialCaps.removeCapability(NET_CAPABILITY_NOT_VPN); - mService.applyUnderlyingCapabilities(new Network[]{}, caps, false); - assertTrue(caps.hasTransport(TRANSPORT_VPN)); - assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); - assertFalse(caps.hasTransport(TRANSPORT_WIFI)); - assertEquals(LINK_BANDWIDTH_UNSPECIFIED, caps.getLinkDownstreamBandwidthKbps()); - assertEquals(LINK_BANDWIDTH_UNSPECIFIED, caps.getLinkUpstreamBandwidthKbps()); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + final NetworkCapabilities withNoUnderlying = new NetworkCapabilities(); + withNoUnderlying.addCapability(NET_CAPABILITY_INTERNET); + withNoUnderlying.addCapability(NET_CAPABILITY_NOT_CONGESTED); + withNoUnderlying.addCapability(NET_CAPABILITY_NOT_ROAMING); + withNoUnderlying.addCapability(NET_CAPABILITY_NOT_SUSPENDED); + withNoUnderlying.addTransportType(TRANSPORT_VPN); + withNoUnderlying.removeCapability(NET_CAPABILITY_NOT_VPN); - NetworkCapabilities otherCaps = new NetworkCapabilities(caps); + final NetworkCapabilities withMobileUnderlying = new NetworkCapabilities(withNoUnderlying); + withMobileUnderlying.addTransportType(TRANSPORT_CELLULAR); + withMobileUnderlying.removeCapability(NET_CAPABILITY_NOT_ROAMING); + withMobileUnderlying.removeCapability(NET_CAPABILITY_NOT_SUSPENDED); + withMobileUnderlying.setLinkDownstreamBandwidthKbps(10); + + final NetworkCapabilities withWifiUnderlying = new NetworkCapabilities(withNoUnderlying); + withWifiUnderlying.addTransportType(TRANSPORT_WIFI); + withWifiUnderlying.addCapability(NET_CAPABILITY_NOT_METERED); + withWifiUnderlying.setLinkUpstreamBandwidthKbps(20); + + final NetworkCapabilities withWifiAndMobileUnderlying = + new NetworkCapabilities(withNoUnderlying); + withWifiAndMobileUnderlying.addTransportType(TRANSPORT_CELLULAR); + withWifiAndMobileUnderlying.addTransportType(TRANSPORT_WIFI); + withWifiAndMobileUnderlying.removeCapability(NET_CAPABILITY_NOT_METERED); + withWifiAndMobileUnderlying.removeCapability(NET_CAPABILITY_NOT_ROAMING); + withWifiAndMobileUnderlying.setLinkDownstreamBandwidthKbps(10); + withWifiAndMobileUnderlying.setLinkUpstreamBandwidthKbps(20); + + NetworkCapabilities caps = new NetworkCapabilities(initialCaps); final boolean notDeclaredMetered = false; - mService.applyUnderlyingCapabilities(new Network[]{null}, otherCaps, notDeclaredMetered); - assertEquals(caps, otherCaps); + mService.applyUnderlyingCapabilities(new Network[]{}, caps, notDeclaredMetered); + assertEquals(withNoUnderlying, caps); + caps = new NetworkCapabilities(initialCaps); + mService.applyUnderlyingCapabilities(new Network[]{null}, caps, notDeclaredMetered); + assertEquals(withNoUnderlying, caps); + + caps = new NetworkCapabilities(initialCaps); mService.applyUnderlyingCapabilities(new Network[]{mobile}, caps, notDeclaredMetered); - assertTrue(caps.hasTransport(TRANSPORT_VPN)); - assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); - assertFalse(caps.hasTransport(TRANSPORT_WIFI)); - assertEquals(10, caps.getLinkDownstreamBandwidthKbps()); - assertEquals(LINK_BANDWIDTH_UNSPECIFIED, caps.getLinkUpstreamBandwidthKbps()); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED)); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertEquals(withMobileUnderlying, caps); mService.applyUnderlyingCapabilities(new Network[]{wifi}, caps, notDeclaredMetered); - assertTrue(caps.hasTransport(TRANSPORT_VPN)); - assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); - assertTrue(caps.hasTransport(TRANSPORT_WIFI)); - assertEquals(LINK_BANDWIDTH_UNSPECIFIED, caps.getLinkDownstreamBandwidthKbps()); - assertEquals(20, caps.getLinkUpstreamBandwidthKbps()); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_METERED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertEquals(withWifiUnderlying, caps); final boolean isDeclaredMetered = true; + withWifiUnderlying.removeCapability(NET_CAPABILITY_NOT_METERED); + caps = new NetworkCapabilities(initialCaps); mService.applyUnderlyingCapabilities(new Network[]{wifi}, caps, isDeclaredMetered); - assertTrue(caps.hasTransport(TRANSPORT_VPN)); - assertFalse(caps.hasTransport(TRANSPORT_CELLULAR)); - assertTrue(caps.hasTransport(TRANSPORT_WIFI)); - assertEquals(LINK_BANDWIDTH_UNSPECIFIED, caps.getLinkDownstreamBandwidthKbps()); - assertEquals(20, caps.getLinkUpstreamBandwidthKbps()); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertEquals(withWifiUnderlying, caps); - mService.applyUnderlyingCapabilities(new Network[]{mobile, wifi}, caps, notDeclaredMetered); - assertTrue(caps.hasTransport(TRANSPORT_VPN)); - assertTrue(caps.hasTransport(TRANSPORT_CELLULAR)); - assertTrue(caps.hasTransport(TRANSPORT_WIFI)); - assertEquals(10, caps.getLinkDownstreamBandwidthKbps()); - assertEquals(20, caps.getLinkUpstreamBandwidthKbps()); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED)); - assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + caps = new NetworkCapabilities(initialCaps); + mService.applyUnderlyingCapabilities(new Network[]{mobile, wifi}, caps, isDeclaredMetered); + assertEquals(withWifiAndMobileUnderlying, caps); - otherCaps = new NetworkCapabilities(caps); + withWifiUnderlying.addCapability(NET_CAPABILITY_NOT_METERED); + caps = new NetworkCapabilities(initialCaps); mService.applyUnderlyingCapabilities(new Network[]{null, mobile, null, wifi}, - otherCaps, notDeclaredMetered); - assertEquals(otherCaps, caps); + caps, notDeclaredMetered); + assertEquals(withWifiAndMobileUnderlying, caps); + + caps = new NetworkCapabilities(initialCaps); + mService.applyUnderlyingCapabilities(new Network[]{null, mobile, null, wifi}, + caps, notDeclaredMetered); + assertEquals(withWifiAndMobileUnderlying, caps); + + mService.applyUnderlyingCapabilities(null, caps, notDeclaredMetered); + assertEquals(withWifiUnderlying, caps); } @Test