From e0fdea3c5e7eb44812ce7344822781cd4d220a0a Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 14 Sep 2022 21:44:22 +0900 Subject: [PATCH] Add a device config for actively preferring bad wifi Test: FrameworksNetTests adb shell device_config put connectivity actively_prefer_bad_wifi + check that dumpsys updates Test: at this patchset tests fail with : Permission denial: reading from settings requires:android.permission.READ_DEVICE_CONFIG Change-Id: Icfe1e64b8313921ff1dc8e4cbd45f967692e1c4a --- .../android/server/ConnectivityService.java | 26 +++-- .../MultinetworkPolicyTracker.java | 102 ++++++++++++++---- .../ConnectivityServiceIntegrationTest.kt | 13 ++- .../server/ConnectivityServiceTest.java | 17 +-- .../MultinetworkPolicyTrackerTest.kt | 29 +++-- ...ltinetworkPolicyTrackerTestDependencies.kt | 47 ++++++++ 6 files changed, 184 insertions(+), 50 deletions(-) create mode 100644 tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTestDependencies.kt diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 1236243fdd..84cf561511 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -5112,22 +5112,32 @@ public class ConnectivityService extends IConnectivityManager.Stub pw.println("Bad Wi-Fi avoidance: " + avoidBadWifi()); pw.increaseIndent(); - pw.println("Config restrict: " + configRestrict); - pw.println("Actively prefer: " + activelyPreferBadWifi()); + pw.println("Config restrict: " + configRestrict); + pw.println("Actively prefer bad wifi: " + activelyPreferBadWifi()); - final String value = mMultinetworkPolicyTracker.getAvoidBadWifiSetting(); + final String settingValue = mMultinetworkPolicyTracker.getAvoidBadWifiSetting(); String description; // Can't use a switch statement because strings are legal case labels, but null is not. - if ("0".equals(value)) { + if ("0".equals(settingValue)) { description = "get stuck"; - } else if (value == null) { + } else if (settingValue == null) { description = "prompt"; - } else if ("1".equals(value)) { + } else if ("1".equals(settingValue)) { description = "avoid"; } else { - description = value + " (?)"; + description = settingValue + " (?)"; } - pw.println("User setting: " + description); + pw.println("Avoid bad wifi setting: " + description); + final Boolean configValue = mMultinetworkPolicyTracker.deviceConfigActivelyPreferBadWifi(); + if (null == configValue) { + description = "unset"; + } else if (configValue) { + description = "force true"; + } else { + description = "force false"; + } + pw.println("Actively prefer bad wifi conf: " + description); + pw.println(); pw.println("Network overrides:"); pw.increaseIndent(); for (NetworkAgentInfo nai : networksSortedById()) { diff --git a/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java b/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java index a36bf4b43c..58196f7fad 100644 --- a/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java +++ b/service/src/com/android/server/connectivity/MultinetworkPolicyTracker.java @@ -32,6 +32,7 @@ import android.net.ConnectivityResources; import android.net.Uri; import android.os.Build; import android.os.Handler; +import android.provider.DeviceConfig; import android.provider.Settings; import android.telephony.SubscriptionManager; import android.telephony.TelephonyCallback; @@ -41,6 +42,7 @@ import android.util.Log; import com.android.connectivity.resources.R; import com.android.internal.annotations.VisibleForTesting; import com.android.modules.utils.build.SdkLevel; +import com.android.net.module.util.DeviceConfigUtils; import java.util.Arrays; import java.util.List; @@ -67,6 +69,9 @@ import java.util.concurrent.RejectedExecutionException; public class MultinetworkPolicyTracker { private static String TAG = MultinetworkPolicyTracker.class.getSimpleName(); + // See Dependencies#getConfigActivelyPreferBadWifi + public static final String CONFIG_ACTIVELY_PREFER_BAD_WIFI = "actively_prefer_bad_wifi"; + private final Context mContext; private final ConnectivityResources mResources; private final Handler mHandler; @@ -81,6 +86,43 @@ public class MultinetworkPolicyTracker { private int mActiveSubId = SubscriptionManager.INVALID_SUBSCRIPTION_ID; private volatile long mTestAllowBadWifiUntilMs = 0; + /** + * Dependencies for testing + */ + @VisibleForTesting + public static class Dependencies { + /** + * @see DeviceConfigUtils#getDeviceConfigPropertyInt + */ + protected int getConfigActivelyPreferBadWifi() { + // CONFIG_ACTIVELY_PREFER_BAD_WIFI is not a feature to be rolled out, but an override + // for tests and an emergency kill switch (which could force the behavior on OR off). + // As such it uses a -1/null/1 scheme, but features should use + // DeviceConfigUtils#isFeatureEnabled instead, to make sure rollbacks disable the + // feature before it's ready on R and before. + return DeviceConfig.getInt(DeviceConfig.NAMESPACE_CONNECTIVITY, + CONFIG_ACTIVELY_PREFER_BAD_WIFI, 0); + } + + /** + @see DeviceConfig#addOnPropertiesChangedListener + */ + protected void addOnDevicePropertiesChangedListener(@NonNull final Executor executor, + @NonNull final DeviceConfig.OnPropertiesChangedListener listener) { + DeviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_CONNECTIVITY, + executor, listener); + } + + @VisibleForTesting + @NonNull + protected Resources getResourcesForActiveSubId( + @NonNull final ConnectivityResources resources, final int activeSubId) { + return SubscriptionManager.getResourcesForSubId( + resources.getResourcesContext(), activeSubId); + } + } + private final Dependencies mDeps; + /** * Whether to prefer bad wifi to a network that yields to bad wifis, even if it never validated * @@ -129,15 +171,17 @@ public class MultinetworkPolicyTracker { } } - public MultinetworkPolicyTracker(Context ctx, Handler handler) { - this(ctx, handler, null); + public MultinetworkPolicyTracker(Context ctx, Handler handler, Runnable avoidBadWifiCallback) { + this(ctx, handler, avoidBadWifiCallback, new Dependencies()); } - public MultinetworkPolicyTracker(Context ctx, Handler handler, Runnable avoidBadWifiCallback) { + public MultinetworkPolicyTracker(Context ctx, Handler handler, Runnable avoidBadWifiCallback, + Dependencies deps) { mContext = ctx; mResources = new ConnectivityResources(ctx); mHandler = handler; mAvoidBadWifiCallback = avoidBadWifiCallback; + mDeps = deps; mSettingsUris = Arrays.asList( Settings.Global.getUriFor(NETWORK_AVOID_BAD_WIFI), Settings.Global.getUriFor(NETWORK_METERED_MULTIPATH_PREFERENCE)); @@ -166,8 +210,11 @@ public class MultinetworkPolicyTracker { mContext.registerReceiverForAllUsers(mBroadcastReceiver, intentFilter, null /* broadcastPermission */, mHandler); + final Executor handlerExecutor = new HandlerExecutor(mHandler); mContext.getSystemService(TelephonyManager.class).registerTelephonyCallback( - new HandlerExecutor(mHandler), new ActiveDataSubscriptionIdListener()); + handlerExecutor, new ActiveDataSubscriptionIdListener()); + mDeps.addOnDevicePropertiesChangedListener(handlerExecutor, + properties -> reevaluateInternal()); reevaluate(); } @@ -201,7 +248,7 @@ public class MultinetworkPolicyTracker { // NETWORK_AVOID_BAD_WIFI setting. if (allowBadWifi) return true; - return getResourcesForActiveSubId() + return mDeps.getResourcesForActiveSubId(mResources, mActiveSubId) .getInteger(R.integer.config_networkAvoidBadWifi) == 0; } @@ -223,7 +270,7 @@ public class MultinetworkPolicyTracker { // bad wifi (only stay stuck on it if already on there). This implementation treats // any non-0 value like 1, on the assumption that anybody setting it non-zero wants // the newer behavior. - return 0 != getResourcesForActiveSubId() + return 0 != mDeps.getResourcesForActiveSubId(mResources, mActiveSubId) .getInteger(R.integer.config_activelyPreferBadWifi); } @@ -237,13 +284,6 @@ public class MultinetworkPolicyTracker { reevaluateInternal(); } - @VisibleForTesting - @NonNull - protected Resources getResourcesForActiveSubId() { - return SubscriptionManager.getResourcesForSubId( - mResources.getResourcesContext(), mActiveSubId); - } - /** * Whether we should display a notification when wifi becomes unvalidated. */ @@ -255,6 +295,29 @@ public class MultinetworkPolicyTracker { return Settings.Global.getString(mResolver, NETWORK_AVOID_BAD_WIFI); } + /** + * Returns whether device config says the device should actively prefer bad wifi. + * + * {@see #configActivelyPrefersBadWifi} for a description of what this does. This device + * config overrides that config overlay. + * + * @return True on Android U and above. + * True if device config says to actively prefer bad wifi. + * False if device config says not to actively prefer bad wifi. + * null if device config doesn't have an opinion (then fall back on the resource). + */ + public Boolean deviceConfigActivelyPreferBadWifi() { + if (SdkLevel.isAtLeastU()) return true; + switch (mDeps.getConfigActivelyPreferBadWifi()) { + case 1: + return Boolean.TRUE; + case -1: + return Boolean.FALSE; + default: + return null; + } + } + @VisibleForTesting public void reevaluate() { mHandler.post(this::reevaluateInternal); @@ -276,7 +339,12 @@ public class MultinetworkPolicyTracker { mAvoidBadWifi = settingAvoidBadWifi || !configRestrictsAvoidBadWifi(); final boolean prevActive = mActivelyPreferBadWifi; - mActivelyPreferBadWifi = configActivelyPrefersBadWifi(); + final Boolean deviceConfigPreferBadWifi = deviceConfigActivelyPreferBadWifi(); + if (null == deviceConfigPreferBadWifi) { + mActivelyPreferBadWifi = configActivelyPrefersBadWifi(); + } else { + mActivelyPreferBadWifi = deviceConfigPreferBadWifi; + } return mAvoidBadWifi != prevAvoid || mActivelyPreferBadWifi != prevActive; } @@ -285,10 +353,8 @@ public class MultinetworkPolicyTracker { * The default (device and carrier-dependent) value for metered multipath preference. */ public int configMeteredMultipathPreference() { - // TODO: use R.integer.config_networkMeteredMultipathPreference directly - final int id = mResources.get().getIdentifier("config_networkMeteredMultipathPreference", - "integer", mResources.getResourcesContext().getPackageName()); - return mResources.get().getInteger(id); + return mDeps.getResourcesForActiveSubId(mResources, mActiveSubId) + .getInteger(R.integer.config_networkMeteredMultipathPreference); } public void updateMeteredMultipathPreference() { diff --git a/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt b/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt index 5b3f37dc82..26b058da4c 100644 --- a/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt +++ b/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt @@ -211,10 +211,15 @@ class ConnectivityServiceIntegrationTest { doReturn(TestNetIdManager()).`when`(deps).makeNetIdManager() doReturn(mock(BpfNetMaps::class.java)).`when`(deps).getBpfNetMaps(any(), any()) doAnswer { inv -> - object : MultinetworkPolicyTracker(inv.getArgument(0), inv.getArgument(1), - inv.getArgument(2)) { - override fun getResourcesForActiveSubId() = resources - } + MultinetworkPolicyTracker(inv.getArgument(0), + inv.getArgument(1), + inv.getArgument(2), + object : MultinetworkPolicyTracker.Dependencies() { + override fun getResourcesForActiveSubId( + connResources: ConnectivityResources, + activeSubId: Int + ) = resources + }) }.`when`(deps).makeMultinetworkPolicyTracker(any(), any(), any()) return deps } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index e1208ef8e9..7993a5cf81 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -29,7 +29,6 @@ import static android.Manifest.permission.NETWORK_FACTORY; import static android.Manifest.permission.NETWORK_SETTINGS; import static android.Manifest.permission.NETWORK_STACK; import static android.Manifest.permission.PACKET_KEEPALIVE_OFFLOAD; -import static android.Manifest.permission.READ_DEVICE_CONFIG; import static android.app.PendingIntent.FLAG_IMMUTABLE; import static android.content.Intent.ACTION_PACKAGE_ADDED; import static android.content.Intent.ACTION_PACKAGE_REMOVED; @@ -366,6 +365,7 @@ import com.android.server.connectivity.CarrierPrivilegeAuthenticator; import com.android.server.connectivity.ClatCoordinator; import com.android.server.connectivity.ConnectivityFlags; import com.android.server.connectivity.MultinetworkPolicyTracker; +import com.android.server.connectivity.MultinetworkPolicyTrackerTestDependencies; import com.android.server.connectivity.Nat464Xlat; import com.android.server.connectivity.NetworkAgentInfo; import com.android.server.connectivity.NetworkNotificationManager; @@ -1634,12 +1634,7 @@ public class ConnectivityServiceTest { volatile int mConfigMeteredMultipathPreference; WrappedMultinetworkPolicyTracker(Context c, Handler h, Runnable r) { - super(c, h, r); - } - - @Override - protected Resources getResourcesForActiveSubId() { - return mResources; + super(c, h, r, new MultinetworkPolicyTrackerTestDependencies(mResources)); } @Override @@ -5810,16 +5805,12 @@ public class ConnectivityServiceTest { public void testPreferBadWifi_doNotPrefer() throws Exception { // Starting with U this mode is no longer supported and can't actually be tested assumeFalse(SdkLevel.isAtLeastU()); - runAsShell(READ_DEVICE_CONFIG, () -> { - doTestPreferBadWifi(false /* preferBadWifi */); - }); + doTestPreferBadWifi(false /* preferBadWifi */); } @Test public void testPreferBadWifi_doPrefer() throws Exception { - runAsShell(READ_DEVICE_CONFIG, () -> { - doTestPreferBadWifi(true /* preferBadWifi */); - }); + doTestPreferBadWifi(true /* preferBadWifi */); } @Test diff --git a/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt b/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt index 83486754f4..b52e8a84e5 100644 --- a/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt +++ b/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTest.kt @@ -27,6 +27,7 @@ import android.net.ConnectivitySettingsManager.NETWORK_METERED_MULTIPATH_PREFERE import com.android.server.connectivity.MultinetworkPolicyTracker.ActiveDataSubscriptionIdListener import android.os.Build import android.os.Handler +import android.os.test.TestLooper import android.provider.Settings import android.telephony.SubscriptionInfo import android.telephony.SubscriptionManager @@ -47,7 +48,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.ArgumentMatchers.anyInt -import org.mockito.ArgumentMatchers.argThat import org.mockito.Mockito.any import org.mockito.Mockito.doCallRealMethod import org.mockito.Mockito.doReturn @@ -55,6 +55,8 @@ import org.mockito.Mockito.mock import org.mockito.Mockito.times import org.mockito.Mockito.verify +const val HANDLER_TIMEOUT_MS = 400 + /** * Tests for [MultinetworkPolicyTracker]. * @@ -91,8 +93,11 @@ class MultinetworkPolicyTrackerTest { Settings.Global.putString(resolver, NETWORK_AVOID_BAD_WIFI, "1") ConnectivityResources.setResourcesContextForTest(it) } - private val handler = mock(Handler::class.java) - private val tracker = MultinetworkPolicyTracker(context, handler) + private val csLooper = TestLooper() + private val handler = Handler(csLooper.looper) + private val trackerDependencies = MultinetworkPolicyTrackerTestDependencies(resources) + private val tracker = MultinetworkPolicyTracker(context, handler, + null /* avoidBadWifiCallback */, trackerDependencies) private fun assertMultipathPreference(preference: Int) { Settings.Global.putString(resolver, NETWORK_METERED_MULTIPATH_PREFERENCE, @@ -146,6 +151,18 @@ class MultinetworkPolicyTrackerTest { } // In all cases, now the system actively prefers bad wifi assertTrue(tracker.activelyPreferBadWifi) + + // Remaining tests are only useful on T-, which support both the old and new mode. + if (SdkLevel.isAtLeastU()) return + + doReturn(0).`when`(resources).getInteger(R.integer.config_activelyPreferBadWifi) + assertTrue(tracker.updateAvoidBadWifi()) + assertFalse(tracker.activelyPreferBadWifi) + + // Simulate update of device config + trackerDependencies.putConfigActivelyPreferBadWifi(1) + csLooper.dispatchAll() + assertTrue(tracker.activelyPreferBadWifi) } @Test @@ -164,6 +181,8 @@ class MultinetworkPolicyTrackerTest { Settings.Global.putString(resolver, NETWORK_METERED_MULTIPATH_PREFERENCE, MULTIPATH_PREFERENCE_PERFORMANCE.toString()) + assertTrue(tracker.avoidBadWifi) + val listenerCaptor = ArgumentCaptor.forClass( ActiveDataSubscriptionIdListener::class.java) verify(telephonyManager, times(1)) @@ -171,10 +190,6 @@ class MultinetworkPolicyTrackerTest { val listener = listenerCaptor.value listener.onActiveDataSubscriptionIdChanged(testSubId) - // Check it get resource value with test sub id. - verify(subscriptionManager, times(1)).getActiveSubscriptionInfo(testSubId) - verify(context).createConfigurationContext(argThat { it.mcc == 310 && it.mnc == 210 }) - // Check if avoidBadWifi and meteredMultipathPreference values have been updated. assertFalse(tracker.avoidBadWifi) assertEquals(MULTIPATH_PREFERENCE_PERFORMANCE, tracker.meteredMultipathPreference) diff --git a/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTestDependencies.kt b/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTestDependencies.kt new file mode 100644 index 0000000000..744c0208ee --- /dev/null +++ b/tests/unit/java/com/android/server/connectivity/MultinetworkPolicyTrackerTestDependencies.kt @@ -0,0 +1,47 @@ +package com.android.server.connectivity + +import android.content.res.Resources +import android.net.ConnectivityResources +import android.provider.DeviceConfig +import android.provider.DeviceConfig.NAMESPACE_CONNECTIVITY +import android.provider.DeviceConfig.OnPropertiesChangedListener +import com.android.internal.annotations.GuardedBy +import com.android.server.connectivity.MultinetworkPolicyTracker.CONFIG_ACTIVELY_PREFER_BAD_WIFI +import java.util.concurrent.Executor + +class MultinetworkPolicyTrackerTestDependencies(private val resources: Resources) : + MultinetworkPolicyTracker.Dependencies() { + @GuardedBy("listeners") + private var configActivelyPreferBadWifi = 0 + // TODO : move this to an actual fake device config object + @GuardedBy("listeners") + private val listeners = mutableListOf>() + + fun putConfigActivelyPreferBadWifi(value: Int) { + synchronized(listeners) { + if (value == configActivelyPreferBadWifi) return + configActivelyPreferBadWifi = value + val p = DeviceConfig.Properties(NAMESPACE_CONNECTIVITY, + mapOf(CONFIG_ACTIVELY_PREFER_BAD_WIFI to value.toString())) + listeners.forEach { (executor, listener) -> + executor.execute { listener.onPropertiesChanged(p) } + } + } + } + + override fun getConfigActivelyPreferBadWifi(): Int { + return synchronized(listeners) { configActivelyPreferBadWifi } + } + + override fun addOnDevicePropertiesChangedListener( + e: Executor, + listener: OnPropertiesChangedListener + ) { + synchronized(listeners) { + listeners.add(e to listener) + } + } + + override fun getResourcesForActiveSubId(res: ConnectivityResources, id: Int): Resources = + resources +}