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

1 files changed, 58 insertions, 25 deletions

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);
         }