Merge changes Ia5bc896c,I0c9406f4,I3108ee94

* changes:
  Make VcnTransportInfoTest pass on AOSP.
  Immediately redact VcnTransportInfo.
  Do not automatically redact TransportInfo objects.

core/java/android/net/vcn/VcnTransportInfo.java

@@ -16,23 +16,17 @@
 
 package android.net.vcn;
 
-import static android.net.NetworkCapabilities.REDACT_ALL;
+import static android.net.NetworkCapabilities.REDACT_NONE;
-import static android.net.NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS;
 import static android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID;
 
-import static com.android.internal.annotations.VisibleForTesting.Visibility.PRIVATE;
-
 import android.annotation.NonNull;
 import android.annotation.Nullable;
-import android.net.NetworkCapabilities;
 import android.net.TransportInfo;
 import android.net.wifi.WifiInfo;
 import android.os.Parcel;
 import android.os.Parcelable;
 import android.telephony.SubscriptionManager;
 
-import com.android.internal.annotations.VisibleForTesting;
-
 import java.util.Objects;
 
 /**
@@ -55,32 +49,17 @@ public class VcnTransportInfo implements TransportInfo, Parcelable {
     @Nullable private final WifiInfo mWifiInfo;
     private final int mSubId;
 
-    /**
-     * The redaction scheme to use when parcelling.
-     *
-     * <p>The TransportInfo/NetworkCapabilities redaction mechanisms rely on redaction being
-     * performed at parcelling time. This means that the redaction scheme must be stored for later
-     * use.
-     *
-     * <p>Since the redaction scheme itself is not parcelled, this field is listed as a transient.
-     *
-     * <p>Defaults to REDACT_ALL when constructed using public constructors, or creating from
-     * parcels.
-     */
-    private final transient long mRedactions;
-
     public VcnTransportInfo(@NonNull WifiInfo wifiInfo) {
-        this(wifiInfo, INVALID_SUBSCRIPTION_ID, REDACT_ALL);
+        this(wifiInfo, INVALID_SUBSCRIPTION_ID);
     }
 
     public VcnTransportInfo(int subId) {
-        this(null /* wifiInfo */, subId, REDACT_ALL);
+        this(null /* wifiInfo */, subId);
     }
 
-    private VcnTransportInfo(@Nullable WifiInfo wifiInfo, int subId, long redactions) {
+    private VcnTransportInfo(@Nullable WifiInfo wifiInfo, int subId) {
         mWifiInfo = wifiInfo;
         mSubId = subId;
-        mRedactions = redactions;
     }
 
     /**
@@ -102,25 +81,14 @@ public class VcnTransportInfo implements TransportInfo, Parcelable {
      * SubscriptionManager#INVALID_SUBSCRIPTION_ID}.
      *
      * @return the Subscription ID if a cellular underlying Network is present, else {@link
-     *     android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID}.
+     *     android.telephony.SubscriptionManager#INVALID_SUBSCRIPTION_ID}.
      */
     public int getSubId() {
         return mSubId;
     }
 
-    /**
-     * Gets the redaction scheme
-     *
-     * @hide
-     */
-    @VisibleForTesting(visibility = PRIVATE)
-    public long getRedaction() {
-        return mRedactions;
-    }
-
     @Override
     public int hashCode() {
-        // mRedactions not hashed, as it is a transient, for control of parcelling
         return Objects.hash(mWifiInfo, mSubId);
     }
 
@@ -128,8 +96,6 @@ public class VcnTransportInfo implements TransportInfo, Parcelable {
     public boolean equals(Object o) {
         if (!(o instanceof VcnTransportInfo)) return false;
         final VcnTransportInfo that = (VcnTransportInfo) o;
-
-        // mRedactions not compared, as it is a transient, for control of parcelling
         return Objects.equals(mWifiInfo, that.mWifiInfo) && mSubId == that.mSubId;
     }
 
@@ -143,31 +109,19 @@ public class VcnTransportInfo implements TransportInfo, Parcelable {
     @NonNull
     public TransportInfo makeCopy(long redactions) {
         return new VcnTransportInfo(
-                mWifiInfo == null ? null : mWifiInfo.makeCopy(redactions), mSubId, redactions);
+                (mWifiInfo == null) ? null : mWifiInfo.makeCopy(redactions), mSubId);
     }
 
     @Override
     public long getApplicableRedactions() {
-        long redactions = REDACT_FOR_NETWORK_SETTINGS;
+        return (mWifiInfo == null) ? REDACT_NONE : mWifiInfo.getApplicableRedactions();
-
-        // Add additional wifi redactions if necessary
-        if (mWifiInfo != null) {
-            redactions |= mWifiInfo.getApplicableRedactions();
-        }
-
-        return redactions;
-    }
-
-    private boolean shouldParcelNetworkSettingsFields() {
-        return (mRedactions & NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS) == 0;
     }
 
     /** {@inheritDoc} */
     @Override
     public void writeToParcel(@NonNull Parcel dest, int flags) {
-        dest.writeInt(shouldParcelNetworkSettingsFields() ? mSubId : INVALID_SUBSCRIPTION_ID);
+        dest.writeInt(mSubId);
-        dest.writeParcelable(
+        dest.writeParcelable(mWifiInfo, flags);
-                shouldParcelNetworkSettingsFields() ? (Parcelable) mWifiInfo : null, flags);
     }
 
     @Override
@@ -181,17 +135,7 @@ public class VcnTransportInfo implements TransportInfo, Parcelable {
                 public VcnTransportInfo createFromParcel(Parcel in) {
                     final int subId = in.readInt();
                     final WifiInfo wifiInfo = in.readParcelable(null);
-
+                    return new VcnTransportInfo(wifiInfo, subId);
-                    // If all fields are their null values, return null TransportInfo to avoid
-                    // leaking information about this being a VCN Network (instead of macro
-                    // cellular, etc)
-                    if (wifiInfo == null && subId == INVALID_SUBSCRIPTION_ID) {
-                        return null;
-                    }
-
-                    // Prevent further forwarding by redacting everything in future parcels from
-                    // this VcnTransportInfo
-                    return new VcnTransportInfo(wifiInfo, subId, REDACT_ALL);
                 }
 
                 public VcnTransportInfo[] newArray(int size) {

packages/Connectivity/framework/src/android/net/NetworkCapabilities.java

@@ -139,19 +139,13 @@ public final class NetworkCapabilities implements Parcelable {
      */
     private String mRequestorPackageName;
 
-    /**
-     * Indicates what fields should be redacted from this instance.
-     */
-    private final @RedactionType long mRedactions;
-
     public NetworkCapabilities() {
-        mRedactions = REDACT_ALL;
         clearAll();
         mNetworkCapabilities = DEFAULT_CAPABILITIES;
     }
 
     public NetworkCapabilities(NetworkCapabilities nc) {
-        this(nc, REDACT_ALL);
+        this(nc, REDACT_NONE);
     }
 
     /**
@@ -163,10 +157,12 @@ public final class NetworkCapabilities implements Parcelable {
      * @hide
      */
     public NetworkCapabilities(@Nullable NetworkCapabilities nc, @RedactionType long redactions) {
-        mRedactions = redactions;
         if (nc != null) {
             set(nc);
         }
+        if (mTransportInfo != null) {
+            mTransportInfo = nc.mTransportInfo.makeCopy(redactions);
+        }
     }
 
     /**
@@ -175,14 +171,6 @@ public final class NetworkCapabilities implements Parcelable {
      * @hide
      */
     public void clearAll() {
-        // Ensures that the internal copies maintained by the connectivity stack does not set it to
-        // anything other than |REDACT_ALL|.
-        if (mRedactions != REDACT_ALL) {
-            // This is needed because the current redaction mechanism relies on redaction while
-            // parceling.
-            throw new UnsupportedOperationException(
-                    "Cannot clear NetworkCapabilities when mRedactions is set");
-        }
         mNetworkCapabilities = mTransportTypes = mForbiddenNetworkCapabilities = 0;
         mLinkUpBandwidthKbps = mLinkDownBandwidthKbps = LINK_BANDWIDTH_UNSPECIFIED;
         mNetworkSpecifier = null;
@@ -211,7 +199,7 @@ public final class NetworkCapabilities implements Parcelable {
         mLinkDownBandwidthKbps = nc.mLinkDownBandwidthKbps;
         mNetworkSpecifier = nc.mNetworkSpecifier;
         if (nc.getTransportInfo() != null) {
-            setTransportInfo(nc.getTransportInfo().makeCopy(mRedactions));
+            setTransportInfo(nc.getTransportInfo());
         } else {
             setTransportInfo(null);
         }

packages/Connectivity/tests/common/java/android/net/NetworkCapabilitiesTest.java

@@ -340,7 +340,7 @@ public class NetworkCapabilitiesTest {
 
     private void testParcelSane(NetworkCapabilities cap) {
         if (isAtLeastS()) {
-            assertParcelSane(cap, 17);
+            assertParcelSane(cap, 16);
         } else if (isAtLeastR()) {
             assertParcelSane(cap, 15);
         } else {

packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java

@@ -19,6 +19,7 @@ package com.android.server;
 import static android.Manifest.permission.CHANGE_NETWORK_STATE;
 import static android.Manifest.permission.CONNECTIVITY_USE_RESTRICTED_NETWORKS;
 import static android.Manifest.permission.DUMP;
+import static android.Manifest.permission.LOCAL_MAC_ADDRESS;
 import static android.Manifest.permission.NETWORK_FACTORY;
 import static android.Manifest.permission.NETWORK_SETTINGS;
 import static android.app.PendingIntent.FLAG_IMMUTABLE;
@@ -9407,9 +9408,9 @@ public class ConnectivityServiceTest {
         @Override
         public TransportInfo makeCopy(@NetworkCapabilities.RedactionType long redactions) {
             return new TestTransportInfo(
-                    (redactions & REDACT_FOR_ACCESS_FINE_LOCATION) != 0,
+                    locationRedacted | (redactions & REDACT_FOR_ACCESS_FINE_LOCATION) != 0,
-                    (redactions & REDACT_FOR_LOCAL_MAC_ADDRESS) != 0,
+                    localMacAddressRedacted | (redactions & REDACT_FOR_LOCAL_MAC_ADDRESS) != 0,
-                    (redactions & REDACT_FOR_NETWORK_SETTINGS) != 0
+                    settingsRedacted | (redactions & REDACT_FOR_NETWORK_SETTINGS) != 0
             );
         }
 
@@ -9432,8 +9433,26 @@ public class ConnectivityServiceTest {
         public int hashCode() {
             return Objects.hash(locationRedacted, localMacAddressRedacted, settingsRedacted);
         }
+
+        @Override
+        public String toString() {
+            return String.format(
+                    "TestTransportInfo{locationRedacted=%s macRedacted=%s settingsRedacted=%s}",
+                    locationRedacted, localMacAddressRedacted, settingsRedacted);
+        }
     }
 
+    private TestTransportInfo getTestTransportInfo(NetworkCapabilities nc) {
+        return (TestTransportInfo) nc.getTransportInfo();
+    }
+
+    private TestTransportInfo getTestTransportInfo(TestNetworkAgentWrapper n) {
+        final NetworkCapabilities nc = mCm.getNetworkCapabilities(n.getNetwork());
+        assertNotNull(nc);
+        return getTestTransportInfo(nc);
+    }
+
+
     private void verifyNetworkCallbackLocationDataInclusionUsingTransportInfoAndOwnerUidInNetCaps(
             @NonNull TestNetworkCallback wifiNetworkCallback, int actualOwnerUid,
             @NonNull TransportInfo actualTransportInfo, int expectedOwnerUid,
@@ -9462,7 +9481,6 @@ public class ConnectivityServiceTest {
         wifiNetworkCallback.expectCapabilitiesThat(mWiFiNetworkAgent,
                 nc -> Objects.equals(expectedOwnerUid, nc.getOwnerUid())
                         && Objects.equals(expectedTransportInfo, nc.getTransportInfo()));
-
     }
 
     @Test
@@ -9483,6 +9501,40 @@ public class ConnectivityServiceTest {
                 wifiNetworkCallack, ownerUid, transportInfo, INVALID_UID, sanitizedTransportInfo);
     }
 
+    @Test
+    public void testTransportInfoRedactionInSynchronousCalls() throws Exception {
+        final NetworkCapabilities ncTemplate = new NetworkCapabilities()
+                .addTransportType(TRANSPORT_WIFI)
+                .setTransportInfo(new TestTransportInfo());
+
+        mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, new LinkProperties(),
+                ncTemplate);
+        mWiFiNetworkAgent.connect(true /* validated; waits for callback */);
+
+        // NETWORK_SETTINGS redaction is controlled by the NETWORK_SETTINGS permission
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).settingsRedacted);
+        withPermission(NETWORK_SETTINGS, () -> {
+            assertFalse(getTestTransportInfo(mWiFiNetworkAgent).settingsRedacted);
+        });
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).settingsRedacted);
+
+        // LOCAL_MAC_ADDRESS redaction is controlled by the LOCAL_MAC_ADDRESS permission
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).localMacAddressRedacted);
+        withPermission(LOCAL_MAC_ADDRESS, () -> {
+            assertFalse(getTestTransportInfo(mWiFiNetworkAgent).localMacAddressRedacted);
+        });
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).localMacAddressRedacted);
+
+        // Synchronous getNetworkCapabilities calls never return unredacted location-sensitive
+        // information.
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).locationRedacted);
+        setupLocationPermissions(Build.VERSION_CODES.S, true, AppOpsManager.OPSTR_FINE_LOCATION,
+                Manifest.permission.ACCESS_FINE_LOCATION);
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).locationRedacted);
+        denyAllLocationPrivilegedPermissions();
+        assertTrue(getTestTransportInfo(mWiFiNetworkAgent).locationRedacted);
+    }
+
     private void setupConnectionOwnerUid(int vpnOwnerUid, @VpnManager.VpnType int vpnType)
             throws Exception {
         final Set<UidRange> vpnRange = Collections.singleton(PRIMARY_UIDRANGE);
@@ -9840,12 +9892,27 @@ public class ConnectivityServiceTest {
         // Connect the cell agent verify that it notifies TestNetworkCallback that it is available
         final TestNetworkCallback callback = new TestNetworkCallback();
         mCm.registerDefaultNetworkCallback(callback);
-        mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR);
+
+        final NetworkCapabilities ncTemplate = new NetworkCapabilities()
+                .addTransportType(TRANSPORT_CELLULAR)
+                .setTransportInfo(new TestTransportInfo());
+        mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, new LinkProperties(),
+                ncTemplate);
         mCellNetworkAgent.connect(true);
         callback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent);
         callback.assertNoCallback();
     }
 
+    private boolean areConnDiagCapsRedacted(NetworkCapabilities nc) {
+        TestTransportInfo ti = (TestTransportInfo) nc.getTransportInfo();
+        return nc.getUids() == null
+                && nc.getAdministratorUids().length == 0
+                && nc.getOwnerUid() == Process.INVALID_UID
+                && getTestTransportInfo(nc).locationRedacted
+                && getTestTransportInfo(nc).localMacAddressRedacted
+                && getTestTransportInfo(nc).settingsRedacted;
+    }
+
     @Test
     public void testConnectivityDiagnosticsCallbackOnConnectivityReportAvailable()
             throws Exception {
@@ -9856,12 +9923,7 @@ public class ConnectivityServiceTest {
 
         // Verify onConnectivityReport fired
         verify(mConnectivityDiagnosticsCallback).onConnectivityReportAvailable(
-                argThat(report -> {
+                argThat(report -> areConnDiagCapsRedacted(report.getNetworkCapabilities())));
-                    final NetworkCapabilities nc = report.getNetworkCapabilities();
-                    return nc.getUids() == null
-                            && nc.getAdministratorUids().length == 0
-                            && nc.getOwnerUid() == Process.INVALID_UID;
-                }));
     }
 
     @Test
@@ -9877,12 +9939,7 @@ public class ConnectivityServiceTest {
 
         // Verify onDataStallSuspected fired
         verify(mConnectivityDiagnosticsCallback).onDataStallSuspected(
-                argThat(report -> {
+                argThat(report -> areConnDiagCapsRedacted(report.getNetworkCapabilities())));
-                    final NetworkCapabilities nc = report.getNetworkCapabilities();
-                    return nc.getUids() == null
-                            && nc.getAdministratorUids().length == 0
-                            && nc.getOwnerUid() == Process.INVALID_UID;
-                }));
     }
 
     @Test

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

@@ -9041,7 +9041,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     private NetworkCapabilities getNetworkCapabilitiesWithoutUids(@NonNull NetworkCapabilities nc) {
-        final NetworkCapabilities sanitized = new NetworkCapabilities(nc);
+        final NetworkCapabilities sanitized = new NetworkCapabilities(nc,
+                NetworkCapabilities.REDACT_ALL);
         sanitized.setUids(null);
         sanitized.setAdministratorUids(new int[0]);
         sanitized.setOwnerUid(Process.INVALID_UID);

tests/vcn/java/android/net/vcn/VcnTransportInfoTest.java

@@ -16,15 +16,17 @@
 
 package android.net.vcn;
 
-import static android.net.NetworkCapabilities.REDACT_ALL;
+import static android.net.NetworkCapabilities.REDACT_FOR_ACCESS_FINE_LOCATION;
-import static android.net.NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS;
+import static android.net.NetworkCapabilities.REDACT_NONE;
 import static android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNull;
 
+import android.net.wifi.WifiConfiguration;
 import android.net.wifi.WifiInfo;
+import android.os.Build;
 import android.os.Parcel;
 
 import org.junit.Test;
@@ -38,12 +40,6 @@ public class VcnTransportInfoTest {
     private static final VcnTransportInfo CELL_UNDERLYING_INFO = new VcnTransportInfo(SUB_ID);
     private static final VcnTransportInfo WIFI_UNDERLYING_INFO = new VcnTransportInfo(WIFI_INFO);
 
-    @Test
-    public void testRedactionDefaults() {
-        assertEquals(REDACT_ALL, CELL_UNDERLYING_INFO.getRedaction());
-        assertEquals(REDACT_ALL, WIFI_UNDERLYING_INFO.getRedaction());
-    }
-
     @Test
     public void testGetWifiInfo() {
         assertEquals(WIFI_INFO, WIFI_UNDERLYING_INFO.getWifiInfo());
@@ -59,15 +55,19 @@ public class VcnTransportInfoTest {
     }
 
     @Test
-    public void testMakeCopySetsRedactions() {
+    public void testMakeCopyRedactForAccessFineLocation() {
         assertEquals(
-                REDACT_FOR_NETWORK_SETTINGS,
+                SUB_ID,
-                ((VcnTransportInfo) CELL_UNDERLYING_INFO.makeCopy(REDACT_FOR_NETWORK_SETTINGS))
+                ((VcnTransportInfo) CELL_UNDERLYING_INFO.makeCopy(REDACT_FOR_ACCESS_FINE_LOCATION))
-                        .getRedaction());
+                        .getSubId());
-        assertEquals(
+
-                REDACT_FOR_NETWORK_SETTINGS,
+        // TODO: remove the if statement when S pushes to AOSP.
-                ((VcnTransportInfo) WIFI_UNDERLYING_INFO.makeCopy(REDACT_FOR_NETWORK_SETTINGS))
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
-                        .getRedaction());
+            assertEquals(
+                    WifiConfiguration.INVALID_NETWORK_ID,
+                    ((VcnTransportInfo) WIFI_UNDERLYING_INFO.makeCopy(
+                            REDACT_FOR_ACCESS_FINE_LOCATION)).getWifiInfo().getNetworkId());
+        }
     }
 
     @Test
@@ -78,35 +78,31 @@ public class VcnTransportInfoTest {
     }
 
     @Test
-    public void testParcelUnparcel() {
+    public void testApplicableRedactions() {
-        verifyParcelingIsNull(CELL_UNDERLYING_INFO);
+        assertEquals(REDACT_NONE, CELL_UNDERLYING_INFO.getApplicableRedactions());
-        verifyParcelingIsNull(WIFI_UNDERLYING_INFO);
-    }
 
-    private void verifyParcelingIsNull(VcnTransportInfo vcnTransportInfo) {
+        final long wifiRedactions = WIFI_INFO.getApplicableRedactions();
-        // Verify redacted by default
+        assertEquals(wifiRedactions, WIFI_UNDERLYING_INFO.getApplicableRedactions());
-        Parcel parcel = Parcel.obtain();
-        vcnTransportInfo.writeToParcel(parcel, 0 /* flags */);
-        parcel.setDataPosition(0);
-
-        assertNull(VcnTransportInfo.CREATOR.createFromParcel(parcel));
     }
 
     @Test
-    public void testParcelUnparcelNotRedactedForSysUi() {
+    public void testParcelNotRedactedForSysUi() {
-        verifyParcelingForSysUi(CELL_UNDERLYING_INFO);
+        VcnTransportInfo cellRedacted = parcelForSysUi(CELL_UNDERLYING_INFO);
-        verifyParcelingForSysUi(WIFI_UNDERLYING_INFO);
+        assertEquals(SUB_ID, cellRedacted.getSubId());
+        VcnTransportInfo wifiRedacted = parcelForSysUi(WIFI_UNDERLYING_INFO);
+        assertEquals(NETWORK_ID, wifiRedacted.getWifiInfo().getNetworkId());
     }
 
-    private void verifyParcelingForSysUi(VcnTransportInfo vcnTransportInfo) {
+    private VcnTransportInfo parcelForSysUi(VcnTransportInfo vcnTransportInfo) {
         // Allow fully unredacted; SysUI will have all the relevant permissions.
-        final VcnTransportInfo unRedacted = (VcnTransportInfo) vcnTransportInfo.makeCopy(0);
+        final VcnTransportInfo unRedacted = (VcnTransportInfo) vcnTransportInfo.makeCopy(
+                REDACT_NONE);
         final Parcel parcel = Parcel.obtain();
         unRedacted.writeToParcel(parcel, 0 /* flags */);
         parcel.setDataPosition(0);
 
         final VcnTransportInfo unparceled = VcnTransportInfo.CREATOR.createFromParcel(parcel);
         assertEquals(vcnTransportInfo, unparceled);
-        assertEquals(REDACT_ALL, unparceled.getRedaction());
+        return unparceled;
     }
 }