From 0819fa72467308af459fd8f25bfb55633efac3a7 Mon Sep 17 00:00:00 2001 From: Automerger Merge Worker Date: Tue, 17 Mar 2020 15:26:02 +0000 Subject: [PATCH 1/2] Make LinkPropertiesTest backwards compatible LinkPropertiesTest must be backwards compatible with Q for CTS. In Q 4 fields were added: DhcpServerAddress, WakeOnLanSupported, CaptivePortalApiUrl, CaptivePortalData. The new test only tests these fields on R and above. testLinkPropertiesParcelable_Q still verifies that there are 14 fields on Q, so the 4 extra fields can be ignored. The changes use androidx.core.os.BuildCompat.isAtLeastR(), so androidx.core_core is added as a dependency to FrameworksNetCommonTests. Test: atest CtsNetTestCasesLatestSdk:android.net.LinkPropertiesTest on Q and R devices Bug: 150918852 Change-Id: I570efa4eb483a717d4204a18473d02653a69f46d Merged-In: I570efa4eb483a717d4204a18473d02653a69f46d (cherry picked from commit b4c981c7c683564dde0693fa05bd02618f7dcba3) --- tests/net/common/Android.bp | 1 + .../java/android/net/LinkPropertiesTest.java | 101 ++++++++++++------ 2 files changed, 72 insertions(+), 30 deletions(-) diff --git a/tests/net/common/Android.bp b/tests/net/common/Android.bp index e44d46088c..46d680fc45 100644 --- a/tests/net/common/Android.bp +++ b/tests/net/common/Android.bp @@ -20,6 +20,7 @@ java_library { name: "FrameworksNetCommonTests", srcs: ["java/**/*.java", "java/**/*.kt"], static_libs: [ + "androidx.core_core", "androidx.test.rules", "junit", "mockito-target-minus-junit4", diff --git a/tests/net/common/java/android/net/LinkPropertiesTest.java b/tests/net/common/java/android/net/LinkPropertiesTest.java index 48b65e592f..2cbe48ce76 100644 --- a/tests/net/common/java/android/net/LinkPropertiesTest.java +++ b/tests/net/common/java/android/net/LinkPropertiesTest.java @@ -29,12 +29,19 @@ import static org.junit.Assert.fail; import android.net.LinkProperties.ProvisioningChange; import android.net.util.LinkPropertiesUtils.CompareResult; +import android.os.Build; import android.system.OsConstants; import android.util.ArraySet; +import androidx.core.os.BuildCompat; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.testutils.DevSdkIgnoreRule; +import com.android.testutils.DevSdkIgnoreRule.IgnoreAfter; +import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo; + +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -50,6 +57,9 @@ import java.util.Set; @RunWith(AndroidJUnit4.class) @SmallTest public class LinkPropertiesTest { + @Rule + public final DevSdkIgnoreRule ignoreRule = new DevSdkIgnoreRule(); + private static final InetAddress ADDRV4 = address("75.208.6.1"); private static final InetAddress ADDRV6 = address("2001:0db8:85a3:0000:0000:8a2e:0370:7334"); private static final InetAddress DNS1 = address("75.208.7.1"); @@ -76,13 +86,23 @@ public class LinkPropertiesTest { private static final LinkAddress LINKADDRV6 = new LinkAddress(ADDRV6, 128); private static final LinkAddress LINKADDRV6LINKLOCAL = new LinkAddress("fe80::1/64"); private static final Uri CAPPORT_API_URL = Uri.parse("https://test.example.com/capportapi"); - private static final CaptivePortalData CAPPORT_DATA = new CaptivePortalData.Builder() - .setVenueInfoUrl(Uri.parse("https://test.example.com/venue")).build(); + + // CaptivePortalData cannot be in a constant as it does not exist on Q. + // The test runner also crashes when scanning for tests if it is a return type. + private static Object getCaptivePortalData() { + return new CaptivePortalData.Builder() + .setVenueInfoUrl(Uri.parse("https://test.example.com/venue")).build(); + } private static InetAddress address(String addrString) { return InetAddresses.parseNumericAddress(addrString); } + private static boolean isAtLeastR() { + // BuildCompat.isAtLeastR is documented to return false on release SDKs (including R) + return Build.VERSION.SDK_INT > Build.VERSION_CODES.Q || BuildCompat.isAtLeastR(); + } + private void checkEmpty(final LinkProperties lp) { assertEquals(0, lp.getAllInterfaceNames().size()); assertEquals(0, lp.getAllAddresses().size()); @@ -98,14 +118,17 @@ public class LinkPropertiesTest { assertNull(lp.getHttpProxy()); assertNull(lp.getTcpBufferSizes()); assertNull(lp.getNat64Prefix()); - assertNull(lp.getDhcpServerAddress()); assertFalse(lp.isProvisioned()); assertFalse(lp.isIpv4Provisioned()); assertFalse(lp.isIpv6Provisioned()); assertFalse(lp.isPrivateDnsActive()); - assertFalse(lp.isWakeOnLanSupported()); - assertNull(lp.getCaptivePortalApiUrl()); - assertNull(lp.getCaptivePortalData()); + + if (isAtLeastR()) { + assertNull(lp.getDhcpServerAddress()); + assertFalse(lp.isWakeOnLanSupported()); + assertNull(lp.getCaptivePortalApiUrl()); + assertNull(lp.getCaptivePortalData()); + } } private LinkProperties makeTestObject() { @@ -127,10 +150,12 @@ public class LinkPropertiesTest { lp.setMtu(MTU); lp.setTcpBufferSizes(TCP_BUFFER_SIZES); lp.setNat64Prefix(new IpPrefix("2001:db8:0:64::/96")); - lp.setDhcpServerAddress(DHCPSERVER); - lp.setWakeOnLanSupported(true); - lp.setCaptivePortalApiUrl(CAPPORT_API_URL); - lp.setCaptivePortalData(CAPPORT_DATA); + if (isAtLeastR()) { + lp.setDhcpServerAddress(DHCPSERVER); + lp.setWakeOnLanSupported(true); + lp.setCaptivePortalApiUrl(CAPPORT_API_URL); + lp.setCaptivePortalData((CaptivePortalData) getCaptivePortalData()); + } return lp; } @@ -169,14 +194,19 @@ public class LinkPropertiesTest { assertTrue(source.isIdenticalTcpBufferSizes(target)); assertTrue(target.isIdenticalTcpBufferSizes(source)); - assertTrue(source.isIdenticalWakeOnLan(target)); - assertTrue(target.isIdenticalWakeOnLan(source)); + if (isAtLeastR()) { + assertTrue(source.isIdenticalDhcpServerAddress(target)); + assertTrue(source.isIdenticalDhcpServerAddress(source)); - assertTrue(source.isIdenticalCaptivePortalApiUrl(target)); - assertTrue(target.isIdenticalCaptivePortalApiUrl(source)); + assertTrue(source.isIdenticalWakeOnLan(target)); + assertTrue(target.isIdenticalWakeOnLan(source)); - assertTrue(source.isIdenticalCaptivePortalData(target)); - assertTrue(target.isIdenticalCaptivePortalData(source)); + assertTrue(source.isIdenticalCaptivePortalApiUrl(target)); + assertTrue(target.isIdenticalCaptivePortalApiUrl(source)); + + assertTrue(source.isIdenticalCaptivePortalData(target)); + assertTrue(target.isIdenticalCaptivePortalData(source)); + } // Check result of equals(). assertTrue(source.equals(target)); @@ -943,8 +973,7 @@ public class LinkPropertiesTest { assertEquals(new ArraySet<>(expectRemoved), (new ArraySet<>(result.removed))); } - @Test - public void testLinkPropertiesParcelable() throws Exception { + private static LinkProperties makeLinkPropertiesForParceling() { LinkProperties source = new LinkProperties(); source.setInterfaceName(NAME); @@ -978,16 +1007,27 @@ public class LinkPropertiesTest { source.setNat64Prefix(new IpPrefix("2001:db8:1:2:64:64::/96")); - source.setWakeOnLanSupported(true); - source.setCaptivePortalApiUrl(CAPPORT_API_URL); - source.setCaptivePortalData(CAPPORT_DATA); - - source.setDhcpServerAddress((Inet4Address) GATEWAY1); - final LinkProperties stacked = new LinkProperties(); stacked.setInterfaceName("test-stacked"); source.addStackedLink(stacked); + return source; + } + + @Test @IgnoreAfter(Build.VERSION_CODES.Q) + public void testLinkPropertiesParcelable_Q() throws Exception { + final LinkProperties source = makeLinkPropertiesForParceling(); + assertParcelSane(source, 14 /* fieldCount */); + } + + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) + public void testLinkPropertiesParcelable() throws Exception { + final LinkProperties source = makeLinkPropertiesForParceling(); + + source.setWakeOnLanSupported(true); + source.setCaptivePortalApiUrl(CAPPORT_API_URL); + source.setCaptivePortalData((CaptivePortalData) getCaptivePortalData()); + source.setDhcpServerAddress((Inet4Address) GATEWAY1); assertParcelSane(source.makeSensitiveFieldsParcelingCopy(), 18 /* fieldCount */); // Verify that without using a sensitiveFieldsParcelingCopy, sensitive fields are cleared. @@ -997,7 +1037,8 @@ public class LinkPropertiesTest { assertEquals(sanitized, parcelingRoundTrip(source)); } - @Test + // Parceling of the scope was broken until Q-QPR2 + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) public void testLinkLocalDnsServerParceling() throws Exception { final String strAddress = "fe80::1%lo"; final LinkProperties lp = new LinkProperties(); @@ -1120,7 +1161,7 @@ public class LinkPropertiesTest { assertFalse(lp.isPrivateDnsActive()); } - @Test + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) public void testDhcpServerAddress() { final LinkProperties lp = makeTestObject(); assertEquals(DHCPSERVER, lp.getDhcpServerAddress()); @@ -1129,7 +1170,7 @@ public class LinkPropertiesTest { assertNull(lp.getDhcpServerAddress()); } - @Test + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) public void testWakeOnLanSupported() { final LinkProperties lp = makeTestObject(); assertTrue(lp.isWakeOnLanSupported()); @@ -1138,7 +1179,7 @@ public class LinkPropertiesTest { assertFalse(lp.isWakeOnLanSupported()); } - @Test + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) public void testCaptivePortalApiUrl() { final LinkProperties lp = makeTestObject(); assertEquals(CAPPORT_API_URL, lp.getCaptivePortalApiUrl()); @@ -1147,10 +1188,10 @@ public class LinkPropertiesTest { assertNull(lp.getCaptivePortalApiUrl()); } - @Test + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) public void testCaptivePortalData() { final LinkProperties lp = makeTestObject(); - assertEquals(CAPPORT_DATA, lp.getCaptivePortalData()); + assertEquals(getCaptivePortalData(), lp.getCaptivePortalData()); lp.clear(); assertNull(lp.getCaptivePortalData()); From fce6aad2beeb2b075c4ef7d775a787c0c6649640 Mon Sep 17 00:00:00 2001 From: Automerger Merge Worker Date: Tue, 17 Mar 2020 16:02:25 +0000 Subject: [PATCH 2/2] Move sensitive field parceling bool to constructor This addresses API review comments recommending to use a copy constructor with additional parameters instead of a dedicated method. makeSensitiveFieldsParcelingCopy becomes LinkProperties(base, true). Bug: 150877475 Test: atest FrameworksNetTests NetworkStackTests NetworkStackNextTests Merged-In: Ib145ca7f36dcbee6ef47d09862a181fc04a28f03 (cherry picked from commit bf091021d332804e875d7fd3424340e32b896cce) Change-Id: I98449430ca5b11f5a62ba43683663bd82650e817 --- core/java/android/net/LinkProperties.java | 30 ++++++++----------- .../android/server/ConnectivityService.java | 2 +- .../java/android/net/LinkPropertiesTest.java | 3 +- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/core/java/android/net/LinkProperties.java b/core/java/android/net/LinkProperties.java index 732ceb560c..fe3f91940e 100644 --- a/core/java/android/net/LinkProperties.java +++ b/core/java/android/net/LinkProperties.java @@ -167,7 +167,19 @@ public final class LinkProperties implements Parcelable { this(source, false /* parcelSensitiveFields */); } - private LinkProperties(@Nullable LinkProperties source, boolean parcelSensitiveFields) { + /** + * Create a copy of a {@link LinkProperties} that may preserve fields that were set + * based on the permissions of the process that originally received it. + * + *

By default {@link LinkProperties} does not preserve such fields during parceling, as + * they should not be shared outside of the process that receives them without appropriate + * checks. + * @param parcelSensitiveFields Whether the sensitive fields should be kept when parceling + * @hide + */ + @SystemApi + @TestApi + public LinkProperties(@Nullable LinkProperties source, boolean parcelSensitiveFields) { mParcelSensitiveFields = parcelSensitiveFields; if (source == null) return; mIfaceName = source.mIfaceName; @@ -1560,22 +1572,6 @@ public final class LinkProperties implements Parcelable { return mCaptivePortalData; } - /** - * Create a copy of this {@link LinkProperties} that will preserve fields that were set - * based on the permissions of the process that received this {@link LinkProperties}. - * - *

By default {@link LinkProperties} does not preserve such fields during parceling, as - * they should not be shared outside of the process that receives them without appropriate - * checks. - * @hide - */ - @SystemApi - @TestApi - @NonNull - public LinkProperties makeSensitiveFieldsParcelingCopy() { - return new LinkProperties(this, true /* parcelSensitiveFields */); - } - /** * Compares this {@code LinkProperties} instance against the target * LinkProperties in {@code obj}. Two LinkPropertieses are equal if diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index deae459e39..8e7cea5ad3 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1710,7 +1710,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } if (checkSettingsPermission(callerPid, callerUid)) { - return lp.makeSensitiveFieldsParcelingCopy(); + return new LinkProperties(lp, true /* parcelSensitiveFields */); } final LinkProperties newLp = new LinkProperties(lp); diff --git a/tests/net/common/java/android/net/LinkPropertiesTest.java b/tests/net/common/java/android/net/LinkPropertiesTest.java index 2cbe48ce76..2b5720a47e 100644 --- a/tests/net/common/java/android/net/LinkPropertiesTest.java +++ b/tests/net/common/java/android/net/LinkPropertiesTest.java @@ -1028,7 +1028,8 @@ public class LinkPropertiesTest { source.setCaptivePortalApiUrl(CAPPORT_API_URL); source.setCaptivePortalData((CaptivePortalData) getCaptivePortalData()); source.setDhcpServerAddress((Inet4Address) GATEWAY1); - assertParcelSane(source.makeSensitiveFieldsParcelingCopy(), 18 /* fieldCount */); + assertParcelSane(new LinkProperties(source, true /* parcelSensitiveFields */), + 18 /* fieldCount */); // Verify that without using a sensitiveFieldsParcelingCopy, sensitive fields are cleared. final LinkProperties sanitized = new LinkProperties(source);