Refactor NetworkStackService for testability

Refactor some methods to allow integration testing between
ConnectivityService and NetworkStack. The integration tests override
some NetworkStack methods to mock permission checks or NetworkMonitor
network requests.

Test: atest NetworkStackTests
Change-Id: Ib5b4458f0b4d1423759e1e4016ab961d3ced7b48

6 files changed, 93 insertions, 48 deletions

diff --git a/Android.bp b/Android.bp
index 9ba4f98..2e71c75 100644
--- a/Android.bp
+++ b/Android.bp
@@ -125,6 +125,12 @@ android_app {
     required: ["NetworkPermissionConfig"],
 }
 
+android_library {
+    name: "TestNetworkStackLib",
+    defaults: ["NetworkStackAppCommon"],
+    manifest: "AndroidManifest.xml",
+}
+
 genrule {
     name: "statslog-networkstack-java-gen",
     tools: ["stats-log-api-gen"],
diff --git a/src/android/net/dhcp/DhcpServer.java b/src/android/net/dhcp/DhcpServer.java
index b8ab94c..f75fe18 100644
--- a/src/android/net/dhcp/DhcpServer.java
+++ b/src/android/net/dhcp/DhcpServer.java
@@ -20,8 +20,6 @@ import static android.net.dhcp.DhcpPacket.DHCP_CLIENT;
 import static android.net.dhcp.DhcpPacket.DHCP_HOST_NAME;
 import static android.net.dhcp.DhcpPacket.DHCP_SERVER;
 import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;
-import static android.net.dhcp.IDhcpServer.STATUS_INVALID_ARGUMENT;
-import static android.net.dhcp.IDhcpServer.STATUS_SUCCESS;
 import static android.net.shared.Inet4AddressUtils.getBroadcastAddress;
 import static android.net.shared.Inet4AddressUtils.getPrefixMaskAsInet4Address;
 import static android.system.OsConstants.AF_INET;
@@ -36,7 +34,7 @@ import static com.android.internal.util.TrafficStatsConstants.TAG_SYSTEM_DHCP_SE
 import static com.android.server.util.NetworkStackConstants.INFINITE_LEASE;
 import static com.android.server.util.NetworkStackConstants.IPV4_ADDR_ALL;
 import static com.android.server.util.NetworkStackConstants.IPV4_ADDR_ANY;
-import static com.android.server.util.PermissionUtil.checkNetworkStackCallingPermission;
+import static com.android.server.util.PermissionUtil.enforceNetworkStackCallingPermission;
 
 import static java.lang.Integer.toUnsignedLong;
 
@@ -213,7 +211,7 @@ public class DhcpServer extends IDhcpServer.Stub {
 
         @Override
         public void checkCaller() {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
         }
     }
 
diff --git a/src/android/net/ip/IpClient.java b/src/android/net/ip/IpClient.java
index 45bdff3..08a9c5c 100644
--- a/src/android/net/ip/IpClient.java
+++ b/src/android/net/ip/IpClient.java
@@ -19,7 +19,7 @@ package android.net.ip;
 import static android.net.RouteInfo.RTN_UNICAST;
 import static android.net.shared.IpConfigurationParcelableUtil.toStableParcelable;
 
-import static com.android.server.util.PermissionUtil.checkNetworkStackCallingPermission;
+import static com.android.server.util.PermissionUtil.enforceNetworkStackCallingPermission;
 
 import android.annotation.NonNull;
 import android.content.Context;
@@ -515,67 +515,67 @@ public class IpClient extends StateMachine {
     class IpClientConnector extends IIpClient.Stub {
         @Override
         public void completedPreDhcpAction() {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.completedPreDhcpAction();
         }
         @Override
         public void confirmConfiguration() {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.confirmConfiguration();
         }
         @Override
         public void readPacketFilterComplete(byte[] data) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.readPacketFilterComplete(data);
         }
         @Override
         public void shutdown() {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.shutdown();
         }
         @Override
         public void startProvisioning(ProvisioningConfigurationParcelable req) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.startProvisioning(ProvisioningConfiguration.fromStableParcelable(req));
         }
         @Override
         public void stop() {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.stop();
         }
         @Override
         public void setL2KeyAndGroupHint(String l2Key, String groupHint) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.setL2KeyAndGroupHint(l2Key, groupHint);
         }
         @Override
         public void setTcpBufferSizes(String tcpBufferSizes) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.setTcpBufferSizes(tcpBufferSizes);
         }
         @Override
         public void setHttpProxy(ProxyInfo proxyInfo) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.setHttpProxy(proxyInfo);
         }
         @Override
         public void setMulticastFilter(boolean enabled) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.setMulticastFilter(enabled);
         }
         @Override
         public void addKeepalivePacketFilter(int slot, TcpKeepalivePacketDataParcelable pkt) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.addKeepalivePacketFilter(slot, pkt);
         }
         @Override
         public void addNattKeepalivePacketFilter(int slot, NattKeepalivePacketDataParcelable pkt) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.addNattKeepalivePacketFilter(slot, pkt);
         }
         @Override
         public void removeKeepalivePacketFilter(int slot) {
-            checkNetworkStackCallingPermission();
+            enforceNetworkStackCallingPermission();
             IpClient.this.removeKeepalivePacketFilter(slot);
         }
 
diff --git a/src/com/android/server/NetworkStackService.java b/src/com/android/server/NetworkStackService.java
index 91cc8c3..99271f7 100644
--- a/src/com/android/server/NetworkStackService.java
+++ b/src/com/android/server/NetworkStackService.java
@@ -21,14 +21,12 @@ import static android.net.dhcp.IDhcpServer.STATUS_SUCCESS;
 import static android.net.dhcp.IDhcpServer.STATUS_UNKNOWN_ERROR;
 
 import static com.android.server.util.PermissionUtil.checkDumpPermission;
-import static com.android.server.util.PermissionUtil.checkNetworkStackCallingPermission;
 
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.app.Service;
 import android.content.Context;
 import android.content.Intent;
-import android.net.ConnectivityManager;
 import android.net.IIpMemoryStore;
 import android.net.IIpMemoryStoreCallbacks;
 import android.net.INetd;
@@ -50,10 +48,13 @@ import android.net.util.SharedLog;
 import android.os.IBinder;
 import android.os.RemoteException;
 
+import androidx.annotation.VisibleForTesting;
+
 import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.IndentingPrintWriter;
 import com.android.server.connectivity.NetworkMonitor;
 import com.android.server.connectivity.ipmemorystore.IpMemoryStoreService;
+import com.android.server.util.PermissionUtil;
 
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
@@ -81,7 +82,8 @@ public class NetworkStackService extends Service {
      */
     public static synchronized IBinder makeConnector(Context context) {
         if (sConnector == null) {
-            sConnector = new NetworkStackConnector(context);
+            sConnector = new NetworkStackConnector(
+                    context, new NetworkStackConnector.PermissionChecker());
         }
         return sConnector;
     }
@@ -103,13 +105,17 @@ public class NetworkStackService extends Service {
         IIpMemoryStore getIpMemoryStoreService();
     }
 
-    private static class NetworkStackConnector extends INetworkStackConnector.Stub
+    /**
+     * Connector implementing INetworkStackConnector for clients.
+     */
+    @VisibleForTesting
+    public static class NetworkStackConnector extends INetworkStackConnector.Stub
             implements NetworkStackServiceManager {
         private static final int NUM_VALIDATION_LOG_LINES = 20;
         private final Context mContext;
+        private final PermissionChecker mPermChecker;
         private final INetd mNetd;
         private final NetworkObserverRegistry mObserverRegistry;
-        private final ConnectivityManager mCm;
         @GuardedBy("mIpClients")
         private final ArrayList<WeakReference<IpClient>> mIpClients = new ArrayList<>();
         private final IpMemoryStoreService mIpMemoryStoreService;
@@ -127,6 +133,19 @@ public class NetworkStackService extends Service {
         /** Whether different versions have been observed on interfaces provided by the system */
         private volatile boolean mConflictingSystemAidlVersions = false;
 
+        /**
+         * Permission checking dependency of the connector, useful for testing.
+         */
+        @VisibleForTesting
+        public static class PermissionChecker {
+            /**
+             * @see PermissionUtil#enforceNetworkStackCallingPermission()
+             */
+            public void enforceNetworkStackCallingPermission() {
+                PermissionUtil.enforceNetworkStackCallingPermission();
+            }
+        }
+
         private SharedLog addValidationLogs(Network network, String name) {
             final SharedLog log = new SharedLog(NUM_VALIDATION_LOG_LINES, network + " - " + name);
             synchronized (mValidationLogs) {
@@ -138,12 +157,14 @@ public class NetworkStackService extends Service {
             return log;
         }
 
-        NetworkStackConnector(Context context) {
+        @VisibleForTesting
+        public NetworkStackConnector(
+                @NonNull Context context, @NonNull PermissionChecker permChecker) {
             mContext = context;
+            mPermChecker = permChecker;
             mNetd = INetd.Stub.asInterface(
                     (IBinder) context.getSystemService(Context.NETD_SERVICE));
             mObserverRegistry = new NetworkObserverRegistry();
-            mCm = context.getSystemService(ConnectivityManager.class);
             mIpMemoryStoreService = new IpMemoryStoreService(context);
 
             try {
@@ -166,7 +187,7 @@ public class NetworkStackService extends Service {
         @Override
         public void makeDhcpServer(@NonNull String ifName, @NonNull DhcpServingParamsParcel params,
                 @NonNull IDhcpServerCallbacks cb) throws RemoteException {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             updateSystemAidlVersion(cb.getInterfaceVersion());
             final DhcpServer server;
             try {
@@ -189,16 +210,16 @@ public class NetworkStackService extends Service {
         @Override
         public void makeNetworkMonitor(Network network, String name, INetworkMonitorCallbacks cb)
                 throws RemoteException {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             updateSystemAidlVersion(cb.getInterfaceVersion());
             final SharedLog log = addValidationLogs(network, name);
             final NetworkMonitor nm = new NetworkMonitor(mContext, cb, network, log);
-            cb.onNetworkMonitorCreated(new NetworkMonitorImpl(nm));
+            cb.onNetworkMonitorCreated(new NetworkMonitorConnector(nm, mPermChecker));
         }
 
         @Override
         public void makeIpClient(String ifName, IIpClientCallbacks cb) throws RemoteException {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             updateSystemAidlVersion(cb.getInterfaceVersion());
             final IpClient ipClient = new IpClient(mContext, ifName, cb, mObserverRegistry, this);
 
@@ -224,7 +245,7 @@ public class NetworkStackService extends Service {
         @Override
         public void fetchIpMemoryStore(@NonNull final IIpMemoryStoreCallbacks cb)
                 throws RemoteException {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             updateSystemAidlVersion(cb.getInterfaceVersion());
             cb.onIpMemoryStoreFetched(mIpMemoryStoreService);
         }
@@ -290,82 +311,94 @@ public class NetworkStackService extends Service {
             fout.println("SystemServerConflicts: " + mConflictingSystemAidlVersions);
         }
 
+        /**
+         * Get the version of the AIDL interface.
+         */
         @Override
         public int getInterfaceVersion() {
             return this.VERSION;
         }
     }
 
-    private static class NetworkMonitorImpl extends INetworkMonitor.Stub {
+    /**
+     * Proxy for {@link NetworkMonitor} that implements {@link INetworkMonitor}.
+     */
+    @VisibleForTesting
+    public static class NetworkMonitorConnector extends INetworkMonitor.Stub {
+        @NonNull
         private final NetworkMonitor mNm;
+        @NonNull
+        private final NetworkStackConnector.PermissionChecker mPermChecker;
 
-        NetworkMonitorImpl(NetworkMonitor nm) {
+        public NetworkMonitorConnector(@NonNull NetworkMonitor nm,
+                @NonNull NetworkStackConnector.PermissionChecker permChecker) {
             mNm = nm;
+            mPermChecker = permChecker;
         }
 
         @Override
         public void start() {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.start();
         }
 
         @Override
         public void launchCaptivePortalApp() {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.launchCaptivePortalApp();
         }
 
         @Override
         public void notifyCaptivePortalAppFinished(int response) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyCaptivePortalAppFinished(response);
         }
 
         @Override
         public void setAcceptPartialConnectivity() {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.setAcceptPartialConnectivity();
         }
 
         @Override
         public void forceReevaluation(int uid) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.forceReevaluation(uid);
         }
 
         @Override
         public void notifyPrivateDnsChanged(PrivateDnsConfigParcel config) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyPrivateDnsSettingsChanged(PrivateDnsConfig.fromParcel(config));
         }
 
         @Override
         public void notifyDnsResponse(int returnCode) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyDnsResponse(returnCode);
         }
 
         @Override
         public void notifyNetworkConnected(LinkProperties lp, NetworkCapabilities nc) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyNetworkConnected(lp, nc);
         }
 
         @Override
         public void notifyNetworkDisconnected() {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyNetworkDisconnected();
         }
 
         @Override
         public void notifyLinkPropertiesChanged(LinkProperties lp) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyLinkPropertiesChanged(lp);
         }
 
         @Override
         public void notifyNetworkCapabilitiesChanged(NetworkCapabilities nc) {
-            checkNetworkStackCallingPermission();
+            mPermChecker.enforceNetworkStackCallingPermission();
             mNm.notifyNetworkCapabilitiesChanged(nc);
         }
 
diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index 6122d98..eea6310 100644
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -379,7 +379,7 @@ public class NetworkMonitor extends StateMachine {
     }
 
     @VisibleForTesting
-    protected NetworkMonitor(Context context, INetworkMonitorCallbacks cb, Network network,
+    public NetworkMonitor(Context context, INetworkMonitorCallbacks cb, Network network,
             IpConnectivityLog logger, SharedLog validationLogs,
             Dependencies deps, DataStallStatsUtils detectionStatsUtils) {
         // Add suffix indicating which NetworkMonitor we're talking about.
@@ -1843,8 +1843,7 @@ public class NetworkMonitor extends StateMachine {
             latencyBroadcast.putExtra(NetworkMonitorUtils.EXTRA_RESPONSE_TIMESTAMP_MS,
                     responseTimestampMs);
         }
-        mContext.sendBroadcastAsUser(latencyBroadcast, UserHandle.CURRENT,
-                NetworkMonitorUtils.PERMISSION_ACCESS_NETWORK_CONDITIONS);
+        mDependencies.sendNetworkConditionsBroadcast(mContext, latencyBroadcast);
     }
 
     private void logNetworkEvent(int evtype) {
@@ -1889,7 +1888,7 @@ public class NetworkMonitor extends StateMachine {
     }
 
     @VisibleForTesting
-    static class Dependencies {
+    public static class Dependencies {
         public Network getPrivateDnsBypassNetwork(Network network) {
             return new OneAddressPerFamilyNetwork(network);
         }
@@ -1948,6 +1947,15 @@ public class NetworkMonitor extends StateMachine {
             return NetworkStackUtils.getDeviceConfigPropertyInt(namespace, name, defaultValue);
         }
 
+        /**
+         * Send a broadcast indicating network conditions.
+         */
+        public void sendNetworkConditionsBroadcast(@NonNull Context context,
+                @NonNull Intent broadcast) {
+            context.sendBroadcastAsUser(broadcast, UserHandle.CURRENT,
+                    NetworkMonitorUtils.PERMISSION_ACCESS_NETWORK_CONDITIONS);
+        }
+
         public static final Dependencies DEFAULT = new Dependencies();
     }
 
diff --git a/src/com/android/server/util/PermissionUtil.java b/src/com/android/server/util/PermissionUtil.java
index c4d736b..28dad25 100644
--- a/src/com/android/server/util/PermissionUtil.java
+++ b/src/com/android/server/util/PermissionUtil.java
@@ -34,7 +34,7 @@ public final class PermissionUtil {
      * Check that the caller is allowed to communicate with the network stack.
      * @throws SecurityException The caller is not allowed to communicate with the network stack.
      */
-    public static void checkNetworkStackCallingPermission() {
+    public static void enforceNetworkStackCallingPermission() {
         final int caller = getCallingUid();
         if (caller == Process.SYSTEM_UID) {
             checkConsistentSystemPid();