From e0fdea3c5e7eb44812ce7344822781cd4d220a0a Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 14 Sep 2022 21:44:22 +0900 Subject: [PATCH 1/2] 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 +} From 11b4d5e8b06595649a8c3ff869b218cc34c293d5 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 30 Sep 2022 14:44:55 +0900 Subject: [PATCH 2/2] Fix a bug where 'avoid unvalidated' is broken After setting 'avoid unvalidated' the next update of the score by the agent will set the wrong value for that flag in the NetworkScore object. Also have the default behavior on S and T be like the old one, rather than the new one Test: manual Change-Id: I0c161fe7098d3c60827fd50190911b6b762dfa42 --- service/ServiceConnectivityResources/res/values/config.xml | 2 +- .../src/com/android/server/connectivity/NetworkAgentInfo.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/service/ServiceConnectivityResources/res/values/config.xml b/service/ServiceConnectivityResources/res/values/config.xml index 7d777ec8f2..22d9b01b0f 100644 --- a/service/ServiceConnectivityResources/res/values/config.xml +++ b/service/ServiceConnectivityResources/res/values/config.xml @@ -97,7 +97,7 @@ On Android 14 and above, the behavior is always like 1, regardless of the value of this setting. --> - 1 + 0 diff --git a/service/src/com/android/server/connectivity/NetworkAgentInfo.java b/service/src/com/android/server/connectivity/NetworkAgentInfo.java index c73217063e..2e92d43452 100644 --- a/service/src/com/android/server/connectivity/NetworkAgentInfo.java +++ b/service/src/com/android/server/connectivity/NetworkAgentInfo.java @@ -1208,7 +1208,7 @@ public class NetworkAgentInfo implements NetworkRanker.Scoreable { */ public void setScore(final NetworkScore score) { mScore = FullScore.fromNetworkScore(score, networkCapabilities, networkAgentConfig, - everValidated(), 0L == getAvoidUnvalidated(), yieldToBadWiFi(), + everValidated(), 0L != getAvoidUnvalidated(), yieldToBadWiFi(), 0L != mFirstEvaluationConcludedTime, isDestroyed()); }