commit ac8977acad045d216432f9ec40cc2425dbd90edf
author Patrick Rohr <prohr@google.com> Wed Mar 09 21:37:14 2022 +0100
committer Patrick Rohr <prohr@google.com> Thu Mar 10 08:41:10 2022 +0100
tree c1f108f4d0056b5ffc73872f24359a2749a152f4
parent 6a77e5d15bc20b776fb906fc3997ea50faff0d9f

Clean up permission validation in EthernetServiceImpl

Test: atest EthernetServiceImplTest
Change-Id: I0ca54e09dd98cab348fc855e8a0bf70a703fffed

service-t/src/com/android/server/ethernet/EthernetServiceImpl.java

diff --git a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java
index 89ac6e4..50b4684 100644
--- a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java
+++ b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java
@@ -215,14 +215,31 @@
                 "EthernetServiceImpl");
     }
 
-    private void validateTestCapabilities(@Nullable final NetworkCapabilities nc) {
-        // For test capabilities, only null or capabilities that include TRANSPORT_TEST are allowed.
+    private void maybeValidateTestCapabilities(final String iface,
+            @Nullable final NetworkCapabilities nc) {
+        if (!mTracker.isValidTestInterface(iface)) {
+            return;
+        }
+        // For test interfaces, only null or capabilities that include TRANSPORT_TEST are
+        // allowed.
         if (nc != null && !nc.hasTransport(TRANSPORT_TEST)) {
             throw new IllegalArgumentException(
                     "Updates to test interfaces must have NetworkCapabilities.TRANSPORT_TEST.");
         }
     }
 
+    private void enforceAdminPermission(final String iface, boolean enforceAutomotive,
+            final String logMessage) {
+        if (mTracker.isValidTestInterface(iface)) {
+            enforceManageTestNetworksPermission();
+        } else {
+            enforceNetworkManagementPermission();
+            if (enforceAutomotive) {
+                enforceAutomotiveDevice(logMessage);
+            }
+        }
+    }
+
     @Override
     public void updateConfiguration(@NonNull final String iface,
             @NonNull final EthernetNetworkUpdateRequest request,
@@ -231,19 +248,11 @@
         Objects.requireNonNull(request);
         throwIfEthernetNotStarted();
 
-        if (mTracker.isValidTestInterface(iface)) {
-            enforceManageTestNetworksPermission();
-            validateTestCapabilities(request.getNetworkCapabilities());
-            // TODO: use NetworkCapabilities#restrictCapabilitiesForTestNetwork when available on a
-            //  local NetworkCapabilities copy to pass to mTracker.updateConfiguration.
-        } else {
-            enforceNetworkManagementPermission();
-            if (request.getNetworkCapabilities() != null) {
-                // only automotive devices are allowed to set the NetworkCapabilities using this API
-                enforceAutomotiveDevice("updateConfiguration() with non-null capabilities");
-            }
-        }
         // TODO: validate that iface is listed in overlay config_ethernet_interfaces
+        // only automotive devices are allowed to set the NetworkCapabilities using this API
+        enforceAdminPermission(iface, request.getNetworkCapabilities() != null,
+                "updateConfiguration() with non-null capabilities");
+        maybeValidateTestCapabilities(iface, request.getNetworkCapabilities());
 
         mTracker.updateConfiguration(
                 iface, request.getIpConfiguration(), request.getNetworkCapabilities(), listener);
@@ -256,13 +265,7 @@
         Objects.requireNonNull(iface);
         throwIfEthernetNotStarted();
 
-        if (mTracker.isValidTestInterface(iface)) {
-            enforceManageTestNetworksPermission();
-        } else {
-            // only automotive devices are allowed to use this API.
-            enforceNetworkManagementPermission();
-            enforceAutomotiveDevice("connectNetwork()");
-        }
+        enforceAdminPermission(iface, true, "connectNetwork()");
 
         mTracker.connectNetwork(iface, listener);
     }
@@ -274,13 +277,7 @@
         Objects.requireNonNull(iface);
         throwIfEthernetNotStarted();
 
-        if (mTracker.isValidTestInterface(iface)) {
-            enforceManageTestNetworksPermission();
-        } else {
-            // only automotive devices are allowed to use this API.
-            enforceNetworkManagementPermission();
-            enforceAutomotiveDevice("disconnectNetwork()");
-        }
+        enforceAdminPermission(iface, true, "connectNetwork()");
 
         mTracker.disconnectNetwork(iface, listener);
     }