From 260e747d200c7b984b16bcfeca992cb3b478276f Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Thu, 3 Dec 2020 21:07:27 +0000 Subject: [PATCH 01/24] Check mode/boost index before accessing cached support value Bug: 174243830 Test: manual Change-Id: I2f81bcd57181791fa95d35ad52e6af2d7d9c8dcf Merged-In: If305a4b694a9d6fcc27da8279f1f53f9fd9cb685 Merged-In: I345d9ff828021da35556f2d51da512840dda8026 (cherry picked from commit 557838f5506760076e5c523081590b2992b619ff) --- ...ndroid_server_power_PowerManagerService.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/services/core/jni/com_android_server_power_PowerManagerService.cpp b/services/core/jni/com_android_server_power_PowerManagerService.cpp index 469838357f306..85d5c1f256930 100644 --- a/services/core/jni/com_android_server_power_PowerManagerService.cpp +++ b/services/core/jni/com_android_server_power_PowerManagerService.cpp @@ -190,19 +190,18 @@ static void setPowerBoostWithHandle(sp handle, Boost boost, int32_t static std::array, static_cast(Boost::DISPLAY_UPDATE_IMMINENT) + 1> boostSupportedArray = {HalSupport::UNKNOWN}; + size_t idx = static_cast(boost); // Quick return if boost is not supported by HAL - if (boost > Boost::DISPLAY_UPDATE_IMMINENT || - boostSupportedArray[static_cast(boost)] == HalSupport::OFF) { + if (idx >= boostSupportedArray.size() || boostSupportedArray[idx] == HalSupport::OFF) { ALOGV("Skipped setPowerBoost %s because HAL doesn't support it", toString(boost).c_str()); return; } - if (boostSupportedArray[static_cast(boost)] == HalSupport::UNKNOWN) { + if (boostSupportedArray[idx] == HalSupport::UNKNOWN) { bool isSupported = false; handle->isBoostSupported(boost, &isSupported); - boostSupportedArray[static_cast(boost)] = - isSupported ? HalSupport::ON : HalSupport::OFF; + boostSupportedArray[idx] = isSupported ? HalSupport::ON : HalSupport::OFF; if (!isSupported) { ALOGV("Skipped setPowerBoost %s because HAL doesn't support it", toString(boost).c_str()); @@ -230,19 +229,18 @@ static bool setPowerModeWithHandle(sp handle, Mode mode, bool enable // Need to increase the array if more mode supported. static std::array, static_cast(Mode::DISPLAY_INACTIVE) + 1> modeSupportedArray = {HalSupport::UNKNOWN}; + size_t idx = static_cast(mode); // Quick return if mode is not supported by HAL - if (mode > Mode::DISPLAY_INACTIVE || - modeSupportedArray[static_cast(mode)] == HalSupport::OFF) { + if (idx >= modeSupportedArray.size() || modeSupportedArray[idx] == HalSupport::OFF) { ALOGV("Skipped setPowerMode %s because HAL doesn't support it", toString(mode).c_str()); return false; } - if (modeSupportedArray[static_cast(mode)] == HalSupport::UNKNOWN) { + if (modeSupportedArray[idx] == HalSupport::UNKNOWN) { bool isSupported = false; handle->isModeSupported(mode, &isSupported); - modeSupportedArray[static_cast(mode)] = - isSupported ? HalSupport::ON : HalSupport::OFF; + modeSupportedArray[idx] = isSupported ? HalSupport::ON : HalSupport::OFF; if (!isSupported) { ALOGV("Skipped setPowerMode %s because HAL doesn't support it", toString(mode).c_str()); return false; From 95602e325ae1ee9cf383db38cb0a8d47567618dc Mon Sep 17 00:00:00 2001 From: Yu-Han Yang Date: Fri, 15 Jan 2021 14:24:09 -0800 Subject: [PATCH 02/24] DO NOT MERGE: Do not inject mock location to chipset Bug: 177561690 Test: on device Change-Id: Icafbdf54fe807f8779377b13cb4e4eb265db692e (cherry picked from commit abccd8f7bc76e19e92d2dfebe2247e97619f7234) --- .../android/server/location/gnss/GnssLocationProvider.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java b/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java index a6a607e4ce66f..8d1d3afab5c5f 100644 --- a/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java @@ -841,6 +841,9 @@ public class GnssLocationProvider extends AbstractLocationProvider implements } private void injectBestLocation(Location location) { + if (location.isFromMockProvider()) { + return; + } if (DEBUG) { Log.d(TAG, "injectBestLocation: " + location); } @@ -942,6 +945,9 @@ public class GnssLocationProvider extends AbstractLocationProvider implements } private void injectLocation(Location location) { + if (location.isFromMockProvider()) { + return; + } if (location.hasAccuracy()) { if (DEBUG) { Log.d(TAG, "injectLocation: " + location); From 0864ae9732240a343fe4fc4c393071964e2e9854 Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 9 Dec 2020 17:00:23 -0800 Subject: [PATCH 03/24] Only allow BROWSABLE && DEFAULT Intents to be always opened Auto verification of app links requires that an intent filter declare action=VIEW, scheme=HTTP(S), category=BROWSABLE. However, PackageManagerService was not taking that into account, missing the category requirement. But the app info Settings UI did take category into account, so it was possible for a user to set an application to automatically open web URIs without understanding that this also granted domains that were not visible in the app info UI. To resolve both this, this change makes it so that both auto verification and the Settings state can only consider the app as "always" open only if the Intent contains both BROWSABLE and DEFAULT. Bug: 175139501 Bug: 175319005 Test: manual, see bug for reproduction steps Merged-In: Ib957258735893bf2779bed19bd400c6726ee6478 Change-Id: Ib957258735893bf2779bed19bd400c6726ee6478 (cherry picked from commit 4266f938c6705466d3844385c2031d1e80ee7b2d) (cherry picked from commit 0f337fcea8de957ac2939f602461913283ecabce) --- .../com/android/server/pm/PackageManagerService.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 0af4ff4df93f7..40723e073ba10 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -35,6 +35,8 @@ import static android.content.Intent.EXTRA_PACKAGE_NAME; import static android.content.Intent.EXTRA_VERSION_CODE; import static android.content.pm.PackageManager.CERT_INPUT_RAW_X509; import static android.content.pm.PackageManager.CERT_INPUT_SHA256; +import static android.content.Intent.CATEGORY_BROWSABLE; +import static android.content.Intent.CATEGORY_DEFAULT; import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DEFAULT; import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DISABLED; import static android.content.pm.PackageManager.COMPONENT_ENABLED_STATE_DISABLED_UNTIL_USED; @@ -7704,6 +7706,13 @@ public class PackageManagerService extends IPackageManager.Stub Slog.i(TAG, " + always: " + info.activityInfo.packageName + " : linkgen=" + linkGeneration); } + + if (!intent.hasCategory(CATEGORY_BROWSABLE) + || !intent.hasCategory(CATEGORY_DEFAULT)) { + undefinedList.add(info); + continue; + } + // Use link-enabled generation as preferredOrder, i.e. // prefer newly-enabled over earlier-enabled. info.preferredOrder = linkGeneration; From a5eb01054b44472d32bda00066dc6ffceb9123d1 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Thu, 3 Dec 2020 15:57:36 +0800 Subject: [PATCH 04/24] Only update native InputApplicationHandle once This makes sure the write operation (NativeInputApplicationHandle ::updateInfo) is always called from window manager side once when calling SurfaceControl.Transaction#setInputWindowInfo or InputManagerService#setFocusedApplication. If the info of input application handle is changed, a new instance will be created. That avoids the race condition of reading the fields of the same InputApplicationInfo instance from input dispatcher. Bug: 171857140 Bug: 161334769 Bug: 174768985 Test: WindowInputTests Merged-In: Ief84bbe6e6fa4da5309912059904932ccf775b75 Merged-In: I70de9835c7699fe6f56fc3655b0fee5c317ecc3a Change-Id: I70de9835c7699fe6f56fc3655b0fee5c317ecc3a (cherry picked from commit a68473ce978f3e3ad73bd127cdcfcf663d9fd4d9) --- .../android/view/InputApplicationHandle.java | 10 +++++++--- core/java/android/view/InputWindowHandle.java | 2 +- ...d_hardware_input_InputApplicationHandle.cpp | 5 +++++ .../com/android/server/wm/ActivityRecord.java | 18 ++++++++++++++++-- .../java/com/android/server/wm/DragState.java | 6 ++---- .../android/server/wm/InputConsumerImpl.java | 6 ++---- .../com/android/server/wm/InputMonitor.java | 14 ++++---------- .../com/android/server/wm/TaskPositioner.java | 6 ++---- .../com/android/server/wm/WindowState.java | 3 ++- 9 files changed, 41 insertions(+), 29 deletions(-) diff --git a/core/java/android/view/InputApplicationHandle.java b/core/java/android/view/InputApplicationHandle.java index 3d05e2a0b9f68..9b96f7fd1c5c3 100644 --- a/core/java/android/view/InputApplicationHandle.java +++ b/core/java/android/view/InputApplicationHandle.java @@ -16,6 +16,7 @@ package android.view; +import android.annotation.NonNull; import android.os.IBinder; /** @@ -31,17 +32,20 @@ public final class InputApplicationHandle { private long ptr; // Application name. - public String name; + public final @NonNull String name; // Dispatching timeout. - public long dispatchingTimeoutNanos; + public final long dispatchingTimeoutNanos; public final IBinder token; private native void nativeDispose(); - public InputApplicationHandle(IBinder token) { + public InputApplicationHandle(@NonNull IBinder token, @NonNull String name, + long dispatchingTimeoutNanos) { this.token = token; + this.name = name; + this.dispatchingTimeoutNanos = dispatchingTimeoutNanos; } public InputApplicationHandle(InputApplicationHandle handle) { diff --git a/core/java/android/view/InputWindowHandle.java b/core/java/android/view/InputWindowHandle.java index 5f74b2a510ca5..71d26b8880f7e 100644 --- a/core/java/android/view/InputWindowHandle.java +++ b/core/java/android/view/InputWindowHandle.java @@ -36,7 +36,7 @@ public final class InputWindowHandle { private long ptr; // The input application handle. - public final InputApplicationHandle inputApplicationHandle; + public InputApplicationHandle inputApplicationHandle; // The token associates input data with a window and its input channel. The client input // channel and the server input channel will both contain this token. diff --git a/core/jni/android_hardware_input_InputApplicationHandle.cpp b/core/jni/android_hardware_input_InputApplicationHandle.cpp index 71edfd553e7ea..c1ecae8618273 100644 --- a/core/jni/android_hardware_input_InputApplicationHandle.cpp +++ b/core/jni/android_hardware_input_InputApplicationHandle.cpp @@ -58,6 +58,11 @@ bool NativeInputApplicationHandle::updateInfo() { if (!obj) { return false; } + if (mInfo.token.get() != nullptr) { + // The java fields are immutable, so it doesn't need to update again. + env->DeleteLocalRef(obj); + return true; + } mInfo.name = getStringField(env, obj, gInputApplicationHandleClassInfo.name, ""); diff --git a/services/core/java/com/android/server/wm/ActivityRecord.java b/services/core/java/com/android/server/wm/ActivityRecord.java index 1a13d0fc64c44..dc4caf8de0fda 100644 --- a/services/core/java/com/android/server/wm/ActivityRecord.java +++ b/services/core/java/com/android/server/wm/ActivityRecord.java @@ -416,7 +416,7 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A // mOccludesParent field. final boolean hasWallpaper; // Input application handle used by the input dispatcher. - final InputApplicationHandle mInputApplicationHandle; + private InputApplicationHandle mInputApplicationHandle; final int launchedFromPid; // always the pid who started the activity. final int launchedFromUid; // always the uid who started the activity. @@ -1501,7 +1501,6 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A info = aInfo; mUserId = UserHandle.getUserId(info.applicationInfo.uid); packageName = info.applicationInfo.packageName; - mInputApplicationHandle = new InputApplicationHandle(appToken); intent = _intent; // If the class name in the intent doesn't match that of the target, this is probably an @@ -1686,6 +1685,21 @@ final class ActivityRecord extends WindowToken implements WindowManagerService.A return lockTaskLaunchMode; } + @NonNull InputApplicationHandle getInputApplicationHandle(boolean update) { + if (mInputApplicationHandle == null) { + mInputApplicationHandle = new InputApplicationHandle(appToken, toString(), + mInputDispatchingTimeoutNanos); + } else if (update) { + final String name = toString(); + if (mInputDispatchingTimeoutNanos != mInputApplicationHandle.dispatchingTimeoutNanos + || !name.equals(mInputApplicationHandle.name)) { + mInputApplicationHandle = new InputApplicationHandle(appToken, name, + mInputDispatchingTimeoutNanos); + } + } + return mInputApplicationHandle; + } + @Override ActivityRecord asActivityRecord() { // I am an activity record! diff --git a/services/core/java/com/android/server/wm/DragState.java b/services/core/java/com/android/server/wm/DragState.java index f840d9273f606..7b562a9f44460 100644 --- a/services/core/java/com/android/server/wm/DragState.java +++ b/services/core/java/com/android/server/wm/DragState.java @@ -269,10 +269,8 @@ class DragState { mInputEventReceiver = new DragInputEventReceiver(mClientChannel, mService.mH.getLooper(), mDragDropController); - mDragApplicationHandle = new InputApplicationHandle(new Binder()); - mDragApplicationHandle.name = "drag"; - mDragApplicationHandle.dispatchingTimeoutNanos = - WindowManagerService.DEFAULT_INPUT_DISPATCHING_TIMEOUT_NANOS; + mDragApplicationHandle = new InputApplicationHandle(new Binder(), "drag", + WindowManagerService.DEFAULT_INPUT_DISPATCHING_TIMEOUT_NANOS); mDragWindowHandle = new InputWindowHandle(mDragApplicationHandle, display.getDisplayId()); diff --git a/services/core/java/com/android/server/wm/InputConsumerImpl.java b/services/core/java/com/android/server/wm/InputConsumerImpl.java index 3b39b6ba18c53..19185736fc892 100644 --- a/services/core/java/com/android/server/wm/InputConsumerImpl.java +++ b/services/core/java/com/android/server/wm/InputConsumerImpl.java @@ -67,10 +67,8 @@ class InputConsumerImpl implements IBinder.DeathRecipient { } mService.mInputManager.registerInputChannel(mServerChannel); - mApplicationHandle = new InputApplicationHandle(new Binder()); - mApplicationHandle.name = name; - mApplicationHandle.dispatchingTimeoutNanos = - WindowManagerService.DEFAULT_INPUT_DISPATCHING_TIMEOUT_NANOS; + mApplicationHandle = new InputApplicationHandle(new Binder(), name, + WindowManagerService.DEFAULT_INPUT_DISPATCHING_TIMEOUT_NANOS); mWindowHandle = new InputWindowHandle(mApplicationHandle, displayId); mWindowHandle.name = name; diff --git a/services/core/java/com/android/server/wm/InputMonitor.java b/services/core/java/com/android/server/wm/InputMonitor.java index 3663ee14ea99e..8366fde24d541 100644 --- a/services/core/java/com/android/server/wm/InputMonitor.java +++ b/services/core/java/com/android/server/wm/InputMonitor.java @@ -43,7 +43,6 @@ import android.os.Trace; import android.os.UserHandle; import android.util.ArrayMap; import android.util.Slog; -import android.view.InputApplicationHandle; import android.view.InputChannel; import android.view.InputEventReceiver; import android.view.InputWindowHandle; @@ -268,6 +267,8 @@ final class InputMonitor { final boolean hasFocus, final boolean hasWallpaper) { // Add a window to our list of input windows. inputWindowHandle.name = child.toString(); + inputWindowHandle.inputApplicationHandle = child.mActivityRecord != null + ? child.mActivityRecord.getInputApplicationHandle(false /* update */) : null; flags = child.getSurfaceTouchableRegion(inputWindowHandle, flags); inputWindowHandle.layoutParamsFlags = flags; inputWindowHandle.layoutParamsType = type; @@ -386,15 +387,8 @@ final class InputMonitor { public void setFocusedAppLw(ActivityRecord newApp) { // Focused app has changed. - if (newApp == null) { - mService.mInputManager.setFocusedApplication(mDisplayId, null); - } else { - final InputApplicationHandle handle = newApp.mInputApplicationHandle; - handle.name = newApp.toString(); - handle.dispatchingTimeoutNanos = newApp.mInputDispatchingTimeoutNanos; - - mService.mInputManager.setFocusedApplication(mDisplayId, handle); - } + mService.mInputManager.setFocusedApplication(mDisplayId, + newApp != null ? newApp.getInputApplicationHandle(true /* update */) : null); } public void pauseDispatchingLw(WindowToken window) { diff --git a/services/core/java/com/android/server/wm/TaskPositioner.java b/services/core/java/com/android/server/wm/TaskPositioner.java index c68b660bb76fd..44f4a58af7ab1 100644 --- a/services/core/java/com/android/server/wm/TaskPositioner.java +++ b/services/core/java/com/android/server/wm/TaskPositioner.java @@ -228,10 +228,8 @@ class TaskPositioner implements IBinder.DeathRecipient { mClientChannel, mService.mAnimationHandler.getLooper(), mService.mAnimator.getChoreographer()); - mDragApplicationHandle = new InputApplicationHandle(new Binder()); - mDragApplicationHandle.name = TAG; - mDragApplicationHandle.dispatchingTimeoutNanos = - WindowManagerService.DEFAULT_INPUT_DISPATCHING_TIMEOUT_NANOS; + mDragApplicationHandle = new InputApplicationHandle(new Binder(), TAG, + WindowManagerService.DEFAULT_INPUT_DISPATCHING_TIMEOUT_NANOS); mDragWindowHandle = new InputWindowHandle(mDragApplicationHandle, displayContent.getDisplayId()); diff --git a/services/core/java/com/android/server/wm/WindowState.java b/services/core/java/com/android/server/wm/WindowState.java index bab83dc6ea705..faa483a48ea4f 100644 --- a/services/core/java/com/android/server/wm/WindowState.java +++ b/services/core/java/com/android/server/wm/WindowState.java @@ -943,7 +943,8 @@ class WindowState extends WindowContainer implements WindowManagerP mLastRequestedHeight = 0; mLayer = 0; mInputWindowHandle = new InputWindowHandle( - mActivityRecord != null ? mActivityRecord.mInputApplicationHandle : null, + mActivityRecord != null + ? mActivityRecord.getInputApplicationHandle(false /* update */) : null, getDisplayId()); // Make sure we initial all fields before adding to parentWindow, to prevent exception From 19845bc16e53fe3436d61e5bc220deb45b507335 Mon Sep 17 00:00:00 2001 From: Muhammad Qureshi Date: Fri, 15 Jan 2021 12:58:50 -0800 Subject: [PATCH 05/24] [RESTRICT AUTOMERGE] Fix potential out of bounds writes in LogEvent. mUidFieldIndex, mExclusiveStateFieldIndex, mAttributionChain{Start|End}Index are stored using int8_t. These values can overflow when mValues.size() exceeds INT8_MAX. Add proper bounds checking to ensure these values don't exceed INT8_MAX. Bug: 174488848 Bug: 174485572 Test: m Test: statsd_test Test: TreeHugger Change-Id: I416e2de06ca2935017e15d4e91000652831e6b6c (cherry picked from commit 53251a491faab1fb88e28b7cafe5913842b8d71d) --- cmds/statsd/src/logd/LogEvent.cpp | 27 ++++++- cmds/statsd/tests/LogEvent_test.cpp | 110 ++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 4 deletions(-) diff --git a/cmds/statsd/src/logd/LogEvent.cpp b/cmds/statsd/src/logd/LogEvent.cpp index f56fa6221bc99..4f031724763f9 100644 --- a/cmds/statsd/src/logd/LogEvent.cpp +++ b/cmds/statsd/src/logd/LogEvent.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include "annotations.h" @@ -216,13 +217,18 @@ void LogEvent::parseAttributionChain(int32_t* pos, int32_t depth, bool* last, last[2] = true; parseString(pos, /*depth=*/2, last, /*numAnnotations=*/0); } - // Check if at least one node was successfully parsed. - if (mValues.size() - 1 > firstUidInChainIndex) { + + if (mValues.size() - 1 > INT8_MAX) { + mValid = false; + } else if (mValues.size() - 1 > firstUidInChainIndex) { + // At least one node was successfully parsed. mAttributionChainStartIndex = static_cast(firstUidInChainIndex); mAttributionChainEndIndex = static_cast(mValues.size() - 1); } - parseAnnotations(numAnnotations, firstUidInChainIndex); + if (mValid) { + parseAnnotations(numAnnotations, firstUidInChainIndex); + } pos[1] = pos[2] = 1; last[1] = last[2] = false; @@ -234,7 +240,8 @@ bool LogEvent::checkPreviousValueType(Type expected) { } void LogEvent::parseIsUidAnnotation(uint8_t annotationType) { - if (mValues.empty() || !checkPreviousValueType(INT) || annotationType != BOOL_TYPE) { + if (mValues.empty() || mValues.size() - 1 > INT8_MAX || !checkPreviousValueType(INT) + || annotationType != BOOL_TYPE) { mValid = false; return; } @@ -270,6 +277,12 @@ void LogEvent::parsePrimaryFieldFirstUidAnnotation(uint8_t annotationType, return; } + if (static_cast(mValues.size() - 1) < firstUidInChainIndex) { // AttributionChain is empty. + mValid = false; + android_errorWriteLog(0x534e4554, "174485572"); + return; + } + const bool primaryField = readNextValue(); mValues[firstUidInChainIndex].mAnnotations.setPrimaryField(primaryField); } @@ -280,6 +293,12 @@ void LogEvent::parseExclusiveStateAnnotation(uint8_t annotationType) { return; } + if (mValues.size() - 1 > INT8_MAX) { + android_errorWriteLog(0x534e4554, "174488848"); + mValid = false; + return; + } + const bool exclusiveState = readNextValue(); mExclusiveStateFieldIndex = static_cast(mValues.size() - 1); mValues[getExclusiveStateFieldIndex()].mAnnotations.setExclusiveState(exclusiveState); diff --git a/cmds/statsd/tests/LogEvent_test.cpp b/cmds/statsd/tests/LogEvent_test.cpp index 5c170c07eb7d9..aed25475da113 100644 --- a/cmds/statsd/tests/LogEvent_test.cpp +++ b/cmds/statsd/tests/LogEvent_test.cpp @@ -363,6 +363,116 @@ TEST(LogEventTest, TestResetStateAnnotation) { EXPECT_EQ(event.getResetState(), resetState); } +TEST(LogEventTest, TestExclusiveStateAnnotationAfterTooManyFields) { + AStatsEvent* event = AStatsEvent_obtain(); + AStatsEvent_setAtomId(event, 100); + + const unsigned int numAttributionNodes = 64; + + uint32_t uids[numAttributionNodes]; + const char* tags[numAttributionNodes]; + + for (unsigned int i = 1; i <= numAttributionNodes; i++) { + uids[i-1] = i; + tags[i-1] = std::to_string(i).c_str(); + } + + AStatsEvent_writeAttributionChain(event, uids, tags, numAttributionNodes); + AStatsEvent_writeInt32(event, 1); + AStatsEvent_addBoolAnnotation(event, ANNOTATION_ID_EXCLUSIVE_STATE, true); + + AStatsEvent_build(event); + + size_t size; + uint8_t* buf = AStatsEvent_getBuffer(event, &size); + + LogEvent logEvent(/*uid=*/1000, /*pid=*/1001); + EXPECT_FALSE(logEvent.parseBuffer(buf, size)); + EXPECT_EQ(-1, logEvent.getExclusiveStateFieldIndex()); + + AStatsEvent_release(event); +} + +TEST(LogEventTest, TestUidAnnotationAfterTooManyFields) { + AStatsEvent* event = AStatsEvent_obtain(); + AStatsEvent_setAtomId(event, 100); + + const unsigned int numAttributionNodes = 64; + + uint32_t uids[numAttributionNodes]; + const char* tags[numAttributionNodes]; + + for (unsigned int i = 1; i <= numAttributionNodes; i++) { + uids[i-1] = i; + tags[i-1] = std::to_string(i).c_str(); + } + + AStatsEvent_writeAttributionChain(event, uids, tags, numAttributionNodes); + AStatsEvent_writeInt32(event, 1); + AStatsEvent_addBoolAnnotation(event, ANNOTATION_ID_IS_UID, true); + + AStatsEvent_build(event); + + size_t size; + uint8_t* buf = AStatsEvent_getBuffer(event, &size); + + LogEvent logEvent(/*uid=*/1000, /*pid=*/1001); + EXPECT_FALSE(logEvent.parseBuffer(buf, size)); + EXPECT_EQ(-1, logEvent.getUidFieldIndex()); + + AStatsEvent_release(event); +} + +TEST(LogEventTest, TestAttributionChainEndIndexAfterTooManyFields) { + AStatsEvent* event = AStatsEvent_obtain(); + AStatsEvent_setAtomId(event, 100); + + const unsigned int numAttributionNodes = 65; + + uint32_t uids[numAttributionNodes]; + const char* tags[numAttributionNodes]; + + for (unsigned int i = 1; i <= numAttributionNodes; i++) { + uids[i-1] = i; + tags[i-1] = std::to_string(i).c_str(); + } + + AStatsEvent_writeAttributionChain(event, uids, tags, numAttributionNodes); + + AStatsEvent_build(event); + + size_t size; + uint8_t* buf = AStatsEvent_getBuffer(event, &size); + + LogEvent logEvent(/*uid=*/1000, /*pid=*/1001); + EXPECT_FALSE(logEvent.parseBuffer(buf, size)); + EXPECT_FALSE(logEvent.hasAttributionChain()); + + AStatsEvent_release(event); +} + +TEST(LogEventTest, TestEmptyAttributionChainWithPrimaryFieldFirstUidAnnotation) { + AStatsEvent* event = AStatsEvent_obtain(); + AStatsEvent_setAtomId(event, 100); + + uint32_t uids[] = {}; + const char* tags[] = {}; + + AStatsEvent_writeInt32(event, 10); + AStatsEvent_writeAttributionChain(event, uids, tags, 0); + AStatsEvent_addBoolAnnotation(event, ANNOTATION_ID_PRIMARY_FIELD_FIRST_UID, true); + + AStatsEvent_build(event); + + size_t size; + uint8_t* buf = AStatsEvent_getBuffer(event, &size); + + LogEvent logEvent(/*uid=*/1000, /*pid=*/1001); + EXPECT_FALSE(logEvent.parseBuffer(buf, size)); + + AStatsEvent_release(event); +} + } // namespace statsd } // namespace os } // namespace android From 459f6cbe7537b1ba0b88b96c44099bc897c1bcc8 Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Fri, 15 Jan 2021 16:15:00 -0800 Subject: [PATCH 06/24] [SettingsProvider] extend font size scale range As per requested in b/176940932#comment3 and b/156260178#comment32, extending the valid font size scale range to [0.25, 5]. Existing CTS tests still pass. BUG: 156260178 Test: atest android.provider.cts.settings.Settings_SystemTest Test: atest android.app.cts.ApplicationTest Change-Id: Icff82d727d63da4353342b0f9a5ca3c2ae1671c1 (cherry picked from commit df8852a0b596f28a4588f553a631fd7122fefac1) Merged-In: Icff82d727d63da4353342b0f9a5ca3c2ae1671c1 (cherry picked from commit bd6fc820a677f5d128149e1152c15ebb20780d33) --- .../provider/settings/validators/SystemSettingsValidators.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java b/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java index cb610fc611421..bcde58494838a 100644 --- a/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java +++ b/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java @@ -89,7 +89,7 @@ public class SystemSettingsValidators { return value == null || value.length() < MAX_LENGTH; } }); - VALIDATORS.put(System.FONT_SCALE, new InclusiveFloatRangeValidator(0.85f, 1.3f)); + VALIDATORS.put(System.FONT_SCALE, new InclusiveFloatRangeValidator(0.25f, 5.0f)); VALIDATORS.put(System.DIM_SCREEN, BOOLEAN_VALIDATOR); VALIDATORS.put( System.DISPLAY_COLOR_MODE, From 70a5fd9979ee7ff2a4071db0335813c0c5a72399 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Thu, 10 Sep 2020 23:20:58 -0700 Subject: [PATCH 07/24] Fix thread safety issue on clearing cache Historically, we havent held a lock on ForceClearCache and ClearCacheIfNecessary. This is not thread safe because kAllPullAtomInfo must be accessed in a lock, especially now that pullers can be registered/unregistered. Test: atest statsd_test, wrote a repro cl. Bug: 168156854 Bug: 173552790 Change-Id: I47d53a6d9d274bca4c78dbfd87e0097091b7b8cb Merged-In: I47d53a6d9d274bca4c78dbfd87e0097091b7b8cb Merged-In: I8bee7a0a6acecc1274d5acc0adb44c5dde8862e4 (cherry picked from commit f9a4bb18ffa1605cced3ee4b161675a0c88941be) (cherry picked from commit db097214b109be7ca1b3ae470d4a8354c98a5b62) --- cmds/statsd/src/external/StatsPullerManager.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmds/statsd/src/external/StatsPullerManager.cpp b/cmds/statsd/src/external/StatsPullerManager.cpp index 8a9ec7456e557..99c39f6017b18 100644 --- a/cmds/statsd/src/external/StatsPullerManager.cpp +++ b/cmds/statsd/src/external/StatsPullerManager.cpp @@ -334,6 +334,7 @@ void StatsPullerManager::OnAlarmFired(int64_t elapsedTimeNs) { } int StatsPullerManager::ForceClearPullerCache() { + std::lock_guard _l(mLock); int totalCleared = 0; for (const auto& pulledAtom : kAllPullAtomInfo) { totalCleared += pulledAtom.second->ForceClearCache(); @@ -342,6 +343,7 @@ int StatsPullerManager::ForceClearPullerCache() { } int StatsPullerManager::ClearPullerCacheIfNecessary(int64_t timestampNs) { + std::lock_guard _l(mLock); int totalCleared = 0; for (const auto& pulledAtom : kAllPullAtomInfo) { totalCleared += pulledAtom.second->ClearCacheIfNecessary(timestampNs); From 8e4928820e972a00342c00cf67e8795a094e6e68 Mon Sep 17 00:00:00 2001 From: Eugene Susla Date: Mon, 26 Oct 2020 09:08:49 -0700 Subject: [PATCH 08/24] RESTRICT AUTOMERGE Prevent non-system overlays from showing over CDM UI Since CDM grants privileges, it should have the same overlay policy as permission UI Test: use an app wit ha visible overlay to ensure the overlay disappears when CDM is shown Fixes: 171221090 Change-Id: I004843edf5a156dc07fb961edf80272f5cb50163 (cherry picked from commit ea4ce4d48eb22e05f03b36e281149f1e944674fa) (cherry picked from commit 3c2fece4a52512c7dc7a0101bc6b435c44490450) --- .../android/companiondevicemanager/DeviceChooserActivity.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/CompanionDeviceManager/src/com/android/companiondevicemanager/DeviceChooserActivity.java b/packages/CompanionDeviceManager/src/com/android/companiondevicemanager/DeviceChooserActivity.java index e501e1269aeb3..5ac059be20106 100644 --- a/packages/CompanionDeviceManager/src/com/android/companiondevicemanager/DeviceChooserActivity.java +++ b/packages/CompanionDeviceManager/src/com/android/companiondevicemanager/DeviceChooserActivity.java @@ -17,6 +17,7 @@ package com.android.companiondevicemanager; import static android.companion.BluetoothDeviceFilterUtils.getDeviceMacAddress; +import static android.view.WindowManager.LayoutParams.SYSTEM_FLAG_HIDE_NON_SYSTEM_OVERLAY_WINDOWS; import static java.util.Objects.requireNonNull; @@ -58,6 +59,8 @@ public class DeviceChooserActivity extends Activity { Log.e(LOG_TAG, "About to show UI, but no devices to show"); } + getWindow().addSystemFlags(SYSTEM_FLAG_HIDE_NON_SYSTEM_OVERLAY_WINDOWS); + if (getService().mRequest.isSingleDevice()) { setContentView(R.layout.device_confirmation); final DeviceFilterPair selectedDevice = getService().mDevicesFound.get(0); From 75419418cfd2f47439d0f65418f4a771cc58d14b Mon Sep 17 00:00:00 2001 From: Eugene Susla Date: Fri, 4 Dec 2020 16:01:46 -0800 Subject: [PATCH 09/24] RESTRICT AUTOMERGE Allow CDM to hide overlays Since CDM has sensitive user consent UIs, it should be able to hide non-system overlays Test: use a 3p overlay app with a visible overlay to ensure overlay disappears when CDM is shown Bug: 171221090 Change-Id: I78673b7b95f4c3cb31fab180201eb17c9123bddf Merged-In: I78673b7b95f4c3cb31fab180201eb17c9123bddf (cherry picked from commit 43e8c61bee99d04f5bb06f883d61fcaeb7debf70) --- packages/CompanionDeviceManager/AndroidManifest.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/CompanionDeviceManager/AndroidManifest.xml b/packages/CompanionDeviceManager/AndroidManifest.xml index ea9b52cbf3d51..e4e5b9fa781a2 100644 --- a/packages/CompanionDeviceManager/AndroidManifest.xml +++ b/packages/CompanionDeviceManager/AndroidManifest.xml @@ -31,6 +31,7 @@ + Date: Mon, 16 Nov 2020 11:22:13 -0500 Subject: [PATCH 10/24] [DO NOT MERGE] Close screenshot process on user switched Currently, we keep the process up even if the user switches, meaning that in some cases (if the user is switched while the screenshot UI is up) we will save images to the wrong profile. This change makes ScreenshotHelper listen for user switches and close the screenshot service, so that a new screenshot is guaranteed to be constructed with the correct user's context. Bug: 170474245 Fix: 170474245 Test: manual -- verified bad state occurs if user switches within the timeout period, ensured that screenshots work immediately after switching with this change. Change-Id: I9d32d0928e6c2bda161d04555438d0dd7afef0ba (cherry picked from commit 7ef1a5dd1506075507412626f2533283d9520144) (cherry picked from commit 05f7aef1b07665aa10aab535613a8457296279a3) --- .../internal/util/ScreenshotHelper.java | 42 ++++++++++++++----- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/core/java/com/android/internal/util/ScreenshotHelper.java b/core/java/com/android/internal/util/ScreenshotHelper.java index a23fc4b57b457..7ee846e9d8c11 100644 --- a/core/java/com/android/internal/util/ScreenshotHelper.java +++ b/core/java/com/android/internal/util/ScreenshotHelper.java @@ -1,12 +1,15 @@ package com.android.internal.util; +import static android.content.Intent.ACTION_USER_SWITCHED; import static android.view.WindowManager.ScreenshotSource.SCREENSHOT_OTHER; import android.annotation.NonNull; import android.annotation.Nullable; +import android.content.BroadcastReceiver; import android.content.ComponentName; import android.content.Context; import android.content.Intent; +import android.content.IntentFilter; import android.content.ServiceConnection; import android.graphics.Insets; import android.graphics.Rect; @@ -161,8 +164,21 @@ public class ScreenshotHelper { private ServiceConnection mScreenshotConnection = null; private final Context mContext; + private final BroadcastReceiver mBroadcastReceiver = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + synchronized (mScreenshotLock) { + if (ACTION_USER_SWITCHED.equals(intent.getAction())) { + resetConnection(); + } + } + } + }; + public ScreenshotHelper(Context context) { mContext = context; + IntentFilter filter = new IntentFilter(ACTION_USER_SWITCHED); + mContext.registerReceiver(mBroadcastReceiver, filter); } /** @@ -279,9 +295,8 @@ public class ScreenshotHelper { final Runnable mScreenshotTimeout = () -> { synchronized (mScreenshotLock) { if (mScreenshotConnection != null) { - mContext.unbindService(mScreenshotConnection); - mScreenshotConnection = null; - mScreenshotService = null; + Log.e(TAG, "Timed out before getting screenshot capture response"); + resetConnection(); notifyScreenshotError(); } } @@ -304,11 +319,7 @@ public class ScreenshotHelper { break; case SCREENSHOT_MSG_PROCESS_COMPLETE: synchronized (mScreenshotLock) { - if (mScreenshotConnection != null) { - mContext.unbindService(mScreenshotConnection); - mScreenshotConnection = null; - mScreenshotService = null; - } + resetConnection(); } break; } @@ -348,9 +359,7 @@ public class ScreenshotHelper { public void onServiceDisconnected(ComponentName name) { synchronized (mScreenshotLock) { if (mScreenshotConnection != null) { - mContext.unbindService(mScreenshotConnection); - mScreenshotConnection = null; - mScreenshotService = null; + resetConnection(); // only log an error if we're still within the timeout period if (handler.hasCallbacks(mScreenshotTimeout)) { handler.removeCallbacks(mScreenshotTimeout); @@ -382,6 +391,17 @@ public class ScreenshotHelper { } } + /** + * Unbinds the current screenshot connection (if any). + */ + private void resetConnection() { + if (mScreenshotConnection != null) { + mContext.unbindService(mScreenshotConnection); + mScreenshotConnection = null; + mScreenshotService = null; + } + } + /** * Notifies the screenshot service to show an error. */ From 41e9055fc37f586f9dde9e9771dd0424995b53c7 Mon Sep 17 00:00:00 2001 From: Joanne Chung Date: Wed, 30 Dec 2020 19:25:39 +0800 Subject: [PATCH 11/24] Get ApplicationInfo using usr id Use ApplicationInfoAsUser() to get the application information by user id to make sure we have correct permission, othewise we will get the SecurityException due to lack of INTERACT_ACROSS_USERS. Bug: 176313819 Test: atest CtsVoiceRecognitionTestCases Test: manual. 1.create profile user 2.Install sample recognizer app 3.Function works fine and no security exception occurred after apply the change Change-Id: I6958dbf661373606fb9dba55e386aaea4399a146 Merged-In: Iaf485537b8082d2109d2134ff987dc7244e31218 (cherry picked from commit d96bcfcdf2f8e9dffaec3a7f12dcee8ea831a99d) --- core/java/android/app/AppOpsManager.java | 12 ++++++------ .../java/com/android/server/appop/AppOpsService.java | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/core/java/android/app/AppOpsManager.java b/core/java/android/app/AppOpsManager.java index c494781d1eeb8..7bfc4603f060b 100644 --- a/core/java/android/app/AppOpsManager.java +++ b/core/java/android/app/AppOpsManager.java @@ -7623,8 +7623,8 @@ public class AppOpsManager { } else if (collectionMode == COLLECT_SYNC // Only collect app-ops when the proxy is trusted && (mContext.checkPermission(Manifest.permission.UPDATE_APP_OPS_STATS, -1, - myUid) == PackageManager.PERMISSION_GRANTED - || isTrustedVoiceServiceProxy(mContext, mContext.getOpPackageName(), op))) { + myUid) == PackageManager.PERMISSION_GRANTED || isTrustedVoiceServiceProxy( + mContext, mContext.getOpPackageName(), op, mContext.getUserId()))) { collectNotedOpSync(op, proxiedAttributionTag); } } @@ -7642,7 +7642,7 @@ public class AppOpsManager { * @hide */ public static boolean isTrustedVoiceServiceProxy(Context context, String packageName, - int code) { + int code, int userId) { // This is a workaround for R QPR, new API change is not allowed. We only allow the current // voice recognizer is also the voice interactor to noteproxy op. if (code != OP_RECORD_AUDIO) { @@ -7654,7 +7654,7 @@ public class AppOpsManager { final String voiceRecognitionServicePackageName = getComponentPackageNameFromString(voiceRecognitionComponent); return (Objects.equals(packageName, voiceRecognitionServicePackageName)) - && isPackagePreInstalled(context, packageName); + && isPackagePreInstalled(context, packageName, userId); } private static String getComponentPackageNameFromString(String from) { @@ -7662,10 +7662,10 @@ public class AppOpsManager { return componentName != null ? componentName.getPackageName() : ""; } - private static boolean isPackagePreInstalled(Context context, String packageName) { + private static boolean isPackagePreInstalled(Context context, String packageName, int userId) { try { final PackageManager pm = context.getPackageManager(); - final ApplicationInfo info = pm.getApplicationInfo(packageName, 0); + final ApplicationInfo info = pm.getApplicationInfoAsUser(packageName, 0, userId); return ((info.flags & ApplicationInfo.FLAG_SYSTEM) != 0); } catch (PackageManager.NameNotFoundException e) { return false; diff --git a/services/core/java/com/android/server/appop/AppOpsService.java b/services/core/java/com/android/server/appop/AppOpsService.java index 4e194e2e495aa..902b2e2228f88 100644 --- a/services/core/java/com/android/server/appop/AppOpsService.java +++ b/services/core/java/com/android/server/appop/AppOpsService.java @@ -3014,8 +3014,8 @@ public class AppOpsService extends IAppOpsService.Stub { // This is a workaround for R QPR, new API change is not allowed. We only allow the current // voice recognizer is also the voice interactor to noteproxy op. - final boolean isTrustVoiceServiceProxy = - AppOpsManager.isTrustedVoiceServiceProxy(mContext, proxyPackageName, code); + final boolean isTrustVoiceServiceProxy = AppOpsManager.isTrustedVoiceServiceProxy(mContext, + proxyPackageName, code, UserHandle.getUserId(proxyUid)); final boolean isSelfBlame = Binder.getCallingUid() == proxiedUid; final boolean isProxyTrusted = mContext.checkPermission( Manifest.permission.UPDATE_APP_OPS_STATS, -1, proxyUid) From d864955916540d87d91344e61f4e05db7b80728b Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 16 Nov 2020 10:11:12 +0900 Subject: [PATCH 12/24] Test a VPN with an underlying network that does not yet exist. This test checks that if a VPN declares an underlying network that does not exist, the capabilities of that network are applied to the VPN as soon as the network starts to exist. Bug: 172870110 Test: test-only change Change-Id: Icc0701cb4cea7d91f7738c1e426e94cd26686b74 Merged-In: Icc0701cb4cea7d91f7738c1e426e94cd26686b74 (cherry picked from commit a21b2252e0b0eccd482b3b69e67b8911b94dba73) --- .../com/android/server/TestNetIdManager.kt | 1 + .../server/ConnectivityServiceTest.java | 57 ++++++++++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/tests/net/integration/util/com/android/server/TestNetIdManager.kt b/tests/net/integration/util/com/android/server/TestNetIdManager.kt index eb290dc7d24ad..938a694e8ba98 100644 --- a/tests/net/integration/util/com/android/server/TestNetIdManager.kt +++ b/tests/net/integration/util/com/android/server/TestNetIdManager.kt @@ -35,4 +35,5 @@ class TestNetIdManager : NetIdManager() { private val nextId = AtomicInteger(MAX_NET_ID) override fun reserveNetId() = nextId.decrementAndGet() override fun releaseNetId(id: Int) = Unit + fun peekNextNetId() = nextId.get() - 1 } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 385005f90c3be..0c1bf548db6dc 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -336,6 +336,7 @@ public class ConnectivityServiceTest { private INetworkPolicyListener mPolicyListener; private WrappedMultinetworkPolicyTracker mPolicyTracker; private HandlerThread mAlarmManagerThread; + private TestNetIdManager mNetIdManager; @Mock IIpConnectivityMetrics mIpConnectivityMetrics; @Mock IpConnectivityMetrics.Logger mMetricsService; @@ -1194,6 +1195,8 @@ public class ConnectivityServiceTest { @Before public void setUp() throws Exception { + mNetIdManager = new TestNetIdManager(); + mContext = InstrumentationRegistry.getContext(); MockitoAnnotations.initMocks(this); @@ -1264,7 +1267,7 @@ public class ConnectivityServiceTest { final ConnectivityService.Dependencies deps = mock(ConnectivityService.Dependencies.class); doReturn(mCsHandlerThread).when(deps).makeHandlerThread(); - doReturn(new TestNetIdManager()).when(deps).makeNetIdManager(); + doReturn(mNetIdManager).when(deps).makeNetIdManager(); doReturn(mNetworkStack).when(deps).getNetworkStack(); doReturn(systemProperties).when(deps).getSystemProperties(); doReturn(mock(ProxyTracker.class)).when(deps).makeProxyTracker(any(), any()); @@ -5186,6 +5189,58 @@ public class ConnectivityServiceTest { assertTrue(lp.getDnsServers().containsAll(dnsServers)); } + @Test + public void testVpnConnectDisconnectUnderlyingNetwork() throws Exception { + final TestNetworkCallback callback = new TestNetworkCallback(); + final NetworkRequest request = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN).build(); + + mCm.registerNetworkCallback(request, callback); + + // Bring up a VPN that specifies an underlying network that does not exist yet. + // Note: it's sort of meaningless for a VPN app to declare a network that doesn't exist yet, + // (and doing so is difficult without using reflection) but it's good to test that the code + // behaves approximately correctly. + final int uid = Process.myUid(); + final TestNetworkAgentWrapper + vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); + final ArraySet ranges = new ArraySet<>(); + ranges.add(new UidRange(uid, uid)); + + final Network wifiNetwork = new Network(mNetIdManager.peekNextNetId()); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); + mMockVpn.setUnderlyingNetworks(new Network[]{wifiNetwork}); + vpnNetworkAgent.connect(false); + mMockVpn.connect(); + callback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); + assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + .hasTransport(TRANSPORT_VPN)); + assertFalse(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + .hasTransport(TRANSPORT_WIFI)); + + // Make that underlying network connect, and expect to see its capabilities immediately + // reflected in the VPN's capabilities. + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + assertEquals(wifiNetwork, mWiFiNetworkAgent.getNetwork()); + mWiFiNetworkAgent.connect(false); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + callback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + .hasTransport(TRANSPORT_VPN)); + assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + .hasTransport(TRANSPORT_WIFI)); + + // Disconnect the network, and expect to see the VPN capabilities change accordingly. + mWiFiNetworkAgent.disconnect(); + callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); + callback.expectCapabilitiesThat(vpnNetworkAgent, (nc) -> + nc.getTransportTypes().length == 1 && nc.hasTransport(TRANSPORT_VPN)); + + vpnNetworkAgent.disconnect(); + mCm.unregisterNetworkCallback(callback); + } + @Test public void testVpnNetworkActive() throws Exception { final int uid = Process.myUid(); From 531c1795567e13b4d55eb447db51561986cbc874 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 16 Nov 2020 16:05:44 +0900 Subject: [PATCH 13/24] Simplify MockVpn. This CL removes four methods in MockVpn by slightly changing the test code to leverage the actual methods implemented by the (production) Vpn superclass. This works because setting mInterface results in isRunningLocked() returning true, which makes a number of methods behave as if the VPN is connected (which is what the test expects). The more realistic behaviour exposes a minor bug in the treatment of underlying networks. Add a TODO to fix it. Bug: 172870110 Test: test-only change Change-Id: I49421183538ba61ca790af71e309ece36b653bf9 Merged-In: I49421183538ba61ca790af71e309ece36b653bf9 (cherry picked from commit 9ededd8476680d40b0b0d5b21e3a4bc0d743adae) --- .../com/android/server/connectivity/Vpn.java | 3 +- .../server/ConnectivityServiceTest.java | 53 ++++++++----------- 2 files changed, 25 insertions(+), 31 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index bcb17bc9e2dc9..9edb0e440baac 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -202,7 +202,8 @@ public class Vpn { @VisibleForTesting protected String mPackage; private int mOwnerUID; private boolean mIsPackageTargetingAtLeastQ; - private String mInterface; + @VisibleForTesting + protected String mInterface; private Connection mConnection; /** Tracks the runners for all VPN types managed by the platform (eg. LegacyVpn, PlatformVpn) */ diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 0c1bf548db6dc..572ea12358ec1 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -319,6 +319,7 @@ public class ConnectivityServiceTest { private static final String MOBILE_IFNAME = "test_rmnet_data0"; private static final String WIFI_IFNAME = "test_wlan0"; private static final String WIFI_WOL_IFNAME = "test_wlan_wol"; + private static final String VPN_IFNAME = "tun10042"; private static final String TEST_PACKAGE_NAME = "com.android.test.package"; private static final String[] EMPTY_STRING_ARRAY = new String[0]; @@ -1033,12 +1034,14 @@ public class ConnectivityServiceTest { public MockVpn(int userId) { super(startHandlerThreadAndReturnLooper(), mServiceContext, mNetworkManagementService, userId, mock(KeyStore.class)); + mConfig = new VpnConfig(); } public void setNetworkAgent(TestNetworkAgentWrapper agent) { agent.waitForIdle(TIMEOUT_MS); mMockNetworkAgent = agent; mNetworkAgent = agent.getNetworkAgent(); + mInterface = VPN_IFNAME; mNetworkCapabilities.set(agent.getNetworkCapabilities()); } @@ -1059,16 +1062,6 @@ public class ConnectivityServiceTest { return mMockNetworkAgent.getNetwork().netId; } - @Override - public boolean appliesToUid(int uid) { - return mConnected; // Trickery to simplify testing. - } - - @Override - protected boolean isCallerEstablishedOwnerLocked() { - return mConnected; // Similar trickery - } - @Override public int getActiveAppVpnType() { return mVpnType; @@ -1077,7 +1070,6 @@ public class ConnectivityServiceTest { private void connect(boolean isAlwaysMetered) { mNetworkCapabilities.set(mMockNetworkAgent.getNetworkCapabilities()); mConnected = true; - mConfig = new VpnConfig(); mConfig.isMetered = isAlwaysMetered; } @@ -1108,7 +1100,6 @@ public class ConnectivityServiceTest { public void disconnect() { mConnected = false; - mConfig = null; } @Override @@ -1121,18 +1112,6 @@ public class ConnectivityServiceTest { private synchronized void setVpnInfo(VpnInfo vpnInfo) { mVpnInfo = vpnInfo; } - - @Override - public synchronized Network[] getUnderlyingNetworks() { - if (mUnderlyingNetworks != null) return mUnderlyingNetworks; - - return super.getUnderlyingNetworks(); - } - - /** Don't override behavior for {@link Vpn#setUnderlyingNetworks}. */ - private synchronized void overrideUnderlyingNetworks(Network[] underlyingNetworks) { - mUnderlyingNetworks = underlyingNetworks; - } } private void mockVpn(int uid) { @@ -5210,7 +5189,7 @@ public class ConnectivityServiceTest { final Network wifiNetwork = new Network(mNetIdManager.peekNextNetId()); mMockVpn.setNetworkAgent(vpnNetworkAgent); mMockVpn.setUids(ranges); - mMockVpn.setUnderlyingNetworks(new Network[]{wifiNetwork}); + mService.setUnderlyingNetworksForVpn(new Network[]{wifiNetwork}); vpnNetworkAgent.connect(false); mMockVpn.connect(); callback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); @@ -5224,8 +5203,17 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); assertEquals(wifiNetwork, mWiFiNetworkAgent.getNetwork()); mWiFiNetworkAgent.connect(false); - callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + // TODO: the callback for the VPN happens before any callbacks are called for the wifi + // network that has just connected. There appear to be two issues here: + // 1. The VPN code will accept an underlying network as soon as getNetworkCapabilities() for + // it returns non-null (which happens very early, during handleRegisterNetworkAgent). + // This is not correct because that that point the network is not connected and cannot + // pass any traffic. + // 2. When a network connects, updateNetworkInfo propagates underlying network capabilities + // before rematching networks. + // Given that this scenario can't really happen, this is probably fine for now. callback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) .hasTransport(TRANSPORT_VPN)); assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) @@ -5289,7 +5277,7 @@ public class ConnectivityServiceTest { vpnNetworkAgent.connect(false); mMockVpn.connect(); - mMockVpn.setUnderlyingNetworks(new Network[0]); + mService.setUnderlyingNetworksForVpn(new Network[0]); genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -7249,16 +7237,21 @@ public class ConnectivityServiceTest { // active final VpnInfo info = new VpnInfo(); info.ownerUid = Process.myUid(); - info.vpnIface = "interface"; + info.vpnIface = VPN_IFNAME; mMockVpn.setVpnInfo(info); - mMockVpn.overrideUnderlyingNetworks(new Network[] {network}); + + final TestNetworkAgentWrapper vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.connect(); + + assertTrue(mService.setUnderlyingNetworksForVpn(new Network[] {network})); assertTrue( "Active VPN permission not applied", mService.checkConnectivityDiagnosticsPermissions( Process.myPid(), Process.myUid(), naiWithoutUid, mContext.getOpPackageName())); - mMockVpn.overrideUnderlyingNetworks(null); + assertTrue(mService.setUnderlyingNetworksForVpn(null)); assertFalse( "VPN shouldn't receive callback on non-underlying network", mService.checkConnectivityDiagnosticsPermissions( From e9b4a895afd89f45546bc82f6fec2aeea8538a8f Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 17 Nov 2020 23:23:58 +0900 Subject: [PATCH 14/24] Increase test coverage for VPN info sent to NetworkStatsService. Bug: 172870110 Test: test-only change Change-Id: I3711b362f31cb92b759e9f5c9d244fb88d9bd5e7 Merged-In: I3711b362f31cb92b759e9f5c9d244fb88d9bd5e7 (cherry picked from commit 04314f147d00660fc5d32613bc06ede3d9ec3b9d) --- .../server/ConnectivityServiceTest.java | 169 +++++++++++++++--- 1 file changed, 149 insertions(+), 20 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 572ea12358ec1..c0293c157c277 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -4745,13 +4745,52 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(networkCallback); } + private void assertSameElementsNoDuplicates(T[] expected, T[] actual) { + // Easier to implement than a proper "assertSameElements" method that also correctly deals + // with duplicates. + final String msg = Arrays.toString(expected) + " != " + Arrays.toString(actual); + assertEquals(msg, expected.length, actual.length); + Set expectedSet = new ArraySet<>(Arrays.asList(expected)); + assertEquals("expected contains duplicates", expectedSet.size(), expected.length); + // actual cannot have duplicates because it's the same length and has the same elements. + Set actualSet = new ArraySet<>(Arrays.asList(actual)); + assertEquals(expectedSet, actualSet); + } + + private void expectForceUpdateIfaces(Network[] networks, String defaultIface, + Integer vpnUid, String vpnIfname, String[] underlyingIfaces) throws Exception { + ArgumentCaptor networksCaptor = ArgumentCaptor.forClass(Network[].class); + ArgumentCaptor vpnInfosCaptor = ArgumentCaptor.forClass(VpnInfo[].class); + + verify(mStatsService, atLeastOnce()).forceUpdateIfaces(networksCaptor.capture(), + any(NetworkState[].class), eq(defaultIface), vpnInfosCaptor.capture()); + + assertSameElementsNoDuplicates(networksCaptor.getValue(), networks); + + VpnInfo[] infos = vpnInfosCaptor.getValue(); + if (vpnUid != null) { + assertEquals("Should have exactly one VPN:", 1, infos.length); + VpnInfo info = infos[0]; + assertEquals("Unexpected VPN owner:", (int) vpnUid, info.ownerUid); + assertEquals("Unexpected VPN interface:", vpnIfname, info.vpnIface); + assertSameElementsNoDuplicates(underlyingIfaces, info.underlyingIfaces); + } else { + assertEquals(0, infos.length); + return; + } + } + + private void expectForceUpdateIfaces(Network[] networks, String defaultIface) throws Exception { + expectForceUpdateIfaces(networks, defaultIface, null, null, new String[0]); + } + @Test public void testStatsIfacesChanged() throws Exception { mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - Network[] onlyCell = new Network[] {mCellNetworkAgent.getNetwork()}; - Network[] onlyWifi = new Network[] {mWiFiNetworkAgent.getNetwork()}; + final Network[] onlyCell = new Network[] {mCellNetworkAgent.getNetwork()}; + final Network[] onlyWifi = new Network[] {mWiFiNetworkAgent.getNetwork()}; LinkProperties cellLp = new LinkProperties(); cellLp.setInterfaceName(MOBILE_IFNAME); @@ -4762,9 +4801,7 @@ public class ConnectivityServiceTest { mCellNetworkAgent.connect(false); mCellNetworkAgent.sendLinkProperties(cellLp); waitForIdle(); - verify(mStatsService, atLeastOnce()) - .forceUpdateIfaces(eq(onlyCell), any(NetworkState[].class), eq(MOBILE_IFNAME), - eq(new VpnInfo[0])); + expectForceUpdateIfaces(onlyCell, MOBILE_IFNAME); reset(mStatsService); // Default network switch should update ifaces. @@ -4772,32 +4809,24 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent.sendLinkProperties(wifiLp); waitForIdle(); assertEquals(wifiLp, mService.getActiveLinkProperties()); - verify(mStatsService, atLeastOnce()) - .forceUpdateIfaces(eq(onlyWifi), any(NetworkState[].class), eq(WIFI_IFNAME), - eq(new VpnInfo[0])); + expectForceUpdateIfaces(onlyWifi, WIFI_IFNAME); reset(mStatsService); // Disconnect should update ifaces. mWiFiNetworkAgent.disconnect(); waitForIdle(); - verify(mStatsService, atLeastOnce()) - .forceUpdateIfaces(eq(onlyCell), any(NetworkState[].class), - eq(MOBILE_IFNAME), eq(new VpnInfo[0])); + expectForceUpdateIfaces(onlyCell, MOBILE_IFNAME); reset(mStatsService); // Metered change should update ifaces mCellNetworkAgent.addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED); waitForIdle(); - verify(mStatsService, atLeastOnce()) - .forceUpdateIfaces(eq(onlyCell), any(NetworkState[].class), eq(MOBILE_IFNAME), - eq(new VpnInfo[0])); + expectForceUpdateIfaces(onlyCell, MOBILE_IFNAME); reset(mStatsService); mCellNetworkAgent.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_METERED); waitForIdle(); - verify(mStatsService, atLeastOnce()) - .forceUpdateIfaces(eq(onlyCell), any(NetworkState[].class), eq(MOBILE_IFNAME), - eq(new VpnInfo[0])); + expectForceUpdateIfaces(onlyCell, MOBILE_IFNAME); reset(mStatsService); // Captive portal change shouldn't update ifaces @@ -4811,9 +4840,101 @@ public class ConnectivityServiceTest { // Roaming change should update ifaces mCellNetworkAgent.addCapability(NetworkCapabilities.NET_CAPABILITY_NOT_ROAMING); waitForIdle(); - verify(mStatsService, atLeastOnce()) - .forceUpdateIfaces(eq(onlyCell), any(NetworkState[].class), eq(MOBILE_IFNAME), - eq(new VpnInfo[0])); + expectForceUpdateIfaces(onlyCell, MOBILE_IFNAME); + reset(mStatsService); + + // Test VPNs. + final LinkProperties lp = new LinkProperties(); + lp.setInterfaceName(VPN_IFNAME); + + final NetworkAgentWrapper vpnNetworkAgent = establishVpnForMyUid(lp); + final Network[] cellAndVpn = new Network[] { + mCellNetworkAgent.getNetwork(), vpnNetworkAgent.getNetwork()}; + Network[] cellAndWifi = new Network[] { + mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; + + // A VPN with default (null) underlying networks sets the underlying network's interfaces... + expectForceUpdateIfaces(cellAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{MOBILE_IFNAME}); + + // ...and updates them as the default network switches. + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(false); + mWiFiNetworkAgent.sendLinkProperties(wifiLp); + final Network[] wifiAndVpn = new Network[] { + mWiFiNetworkAgent.getNetwork(), vpnNetworkAgent.getNetwork()}; + cellAndWifi = new Network[] { + mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; + + waitForIdle(); + assertEquals(wifiLp, mService.getActiveLinkProperties()); + expectForceUpdateIfaces(wifiAndVpn, WIFI_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{WIFI_IFNAME}); + reset(mStatsService); + + // A VPN that sets its underlying networks passes the underlying interfaces, and influences + // the default interface sent to NetworkStatsService by virtue of applying to the system + // server UID (or, in this test, to the test's UID). This is the reason for sending + // MOBILE_IFNAME even though the default network is wifi. + // TODO: fix this to pass in the actual default network interface. Whether or not the VPN + // applies to the system server UID should not have any bearing on network stats. + mService.setUnderlyingNetworksForVpn(onlyCell); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{MOBILE_IFNAME}); + reset(mStatsService); + + mService.setUnderlyingNetworksForVpn(cellAndWifi); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{MOBILE_IFNAME, WIFI_IFNAME}); + reset(mStatsService); + + // If an underlying network disconnects, that interface should no longer be underlying. + // This doesn't actually work because disconnectAndDestroyNetwork only notifies + // NetworkStatsService before the underlying network is actually removed. So the underlying + // network will only be removed if notifyIfacesChangedForNetworkStats is called again. This + // could result in incorrect data usage measurements if the interface used by the + // disconnected network is reused by a system component that does not register an agent for + // it (e.g., tethering). + mCellNetworkAgent.disconnect(); + waitForIdle(); + assertNull(mService.getLinkProperties(mCellNetworkAgent.getNetwork())); + expectForceUpdateIfaces(wifiAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{MOBILE_IFNAME, WIFI_IFNAME}); + + // Confirm that we never tell NetworkStatsService that cell is no longer the underlying + // network for the VPN... + verify(mStatsService, never()).forceUpdateIfaces(any(Network[].class), + any(NetworkState[].class), any() /* anyString() doesn't match null */, + argThat(infos -> infos[0].underlyingIfaces.length == 1 + && WIFI_IFNAME.equals(infos[0].underlyingIfaces[0]))); + verifyNoMoreInteractions(mStatsService); + reset(mStatsService); + + // ... but if something else happens that causes notifyIfacesChangedForNetworkStats to be + // called again, it does. For example, connect Ethernet, but with a low score, such that it + // does not become the default network. + mEthernetNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_ETHERNET); + mEthernetNetworkAgent.adjustScore(-40); + mEthernetNetworkAgent.connect(false); + waitForIdle(); + verify(mStatsService).forceUpdateIfaces(any(Network[].class), + any(NetworkState[].class), any() /* anyString() doesn't match null */, + argThat(vpnInfos -> vpnInfos[0].underlyingIfaces.length == 1 + && WIFI_IFNAME.equals(vpnInfos[0].underlyingIfaces[0]))); + mEthernetNetworkAgent.disconnect(); + reset(mStatsService); + + // When a VPN declares no underlying networks (i.e., no connectivity), getAllVpnInfo + // does not return the VPN, so CS does not pass it to NetworkStatsService. This causes + // NetworkStatsFactory#adjustForTunAnd464Xlat not to attempt any VPN data migration, which + // is probably a performance improvement (though it's very unlikely that a VPN would declare + // no underlying networks). + // Also, for the same reason as above, the active interface passed in is null. + mService.setUnderlyingNetworksForVpn(new Network[0]); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, null); reset(mStatsService); } @@ -7052,6 +7173,14 @@ public class ConnectivityServiceTest { return vpnNetworkAgent; } + private TestNetworkAgentWrapper establishVpnForMyUid(LinkProperties lp) + throws Exception { + final int uid = Process.myUid(); + final ArraySet ranges = new ArraySet<>(); + ranges.add(new UidRange(uid, uid)); + return establishVpn(lp, uid, ranges); + } + private static PackageInfo buildPackageInfo(boolean hasSystemPermission, int uid) { final PackageInfo packageInfo = new PackageInfo(); if (hasSystemPermission) { From d8689463cf93bc72f6cdab8a15c65d846bfd6ec8 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 18 Nov 2020 22:50:57 +0900 Subject: [PATCH 15/24] Make MockVpn more realistic and easier to use. MockVpn is very difficult to use because it requires the test caller keeping track of both the MockVpn object and an accompanying TestNetworkAgentWrapper. It's also not very realistic: for example, connect() doesn't actually connect anything, it just makes it so that if ConnectivityService tries to update the capabilities, the attempt will not be ignored. Also, unlike the real code in Vpn, it connects with empty NetworkCapabilities (in particular, with empty UID ranges). Make this easier to use and a bit more realistic by: - Allowing TestNetworkAgentWrapper to take a "NetworkCapabilities template" that will form the initial capabilities sent when the agent registers with ConnectivityService. This allows the VPN to register its agent with its UID ranges already set, like the production code does. - Providing separate methods to register the NetworkAgent and mark it connected for cases where the test needs to make changes to the NetworkAgent before connecting (e.g., poking NetworkMonitor). - Putting the TestNetworkAgentWrapper inside MockVpn and driving it through MockVpn's methods. In order not to have too many wrapper functions (and because we can't delegate like in Kotlin), there's still an agent() method that returns the TestNetworkAgentWrapper. Bug: 172870110 Test: test-only change Change-Id: I749ff325bc13ac96f512270b86d1f67686eec378 Merged-In: I749ff325bc13ac96f512270b86d1f67686eec378 (cherry picked from commit a083cb1ffed7a851f2748463a31904fc4c6b625d) --- .../ConnectivityServiceIntegrationTest.kt | 5 +- .../android/server/NetworkAgentWrapper.java | 6 +- .../server/ConnectivityServiceTest.java | 514 ++++++++---------- 3 files changed, 227 insertions(+), 298 deletions(-) diff --git a/tests/net/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt b/tests/net/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt index c4801aab5c321..344d0c3260c9a 100644 --- a/tests/net/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt +++ b/tests/net/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt @@ -195,7 +195,8 @@ class ConnectivityServiceIntegrationTest { "https://secure.test.android.com", responseCode = 204, contentLength = 42, redirectUrl = null)) - val na = NetworkAgentWrapper(TRANSPORT_CELLULAR, LinkProperties(), context) + val na = NetworkAgentWrapper(TRANSPORT_CELLULAR, LinkProperties(), null /* ncTemplate */, + context) networkStackClient.verifyNetworkMonitorCreated(na.network, TEST_TIMEOUT_MS) na.addCapability(NET_CAPABILITY_INTERNET) @@ -204,4 +205,4 @@ class ConnectivityServiceIntegrationTest { testCallback.expectAvailableThenValidatedCallbacks(na.network, TEST_TIMEOUT_MS) assertEquals(2, nsInstrumentation.getRequestUrls().size) } -} \ No newline at end of file +} diff --git a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java index 0ffafd45613a8..03954de984386 100644 --- a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java +++ b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java @@ -69,12 +69,12 @@ public class NetworkAgentWrapper implements TestableNetworkCallback.HasNetwork { private int mStopKeepaliveError = SocketKeepalive.NO_KEEPALIVE; private Integer mExpectedKeepaliveSlot = null; - public NetworkAgentWrapper(int transport, LinkProperties linkProperties, Context context) - throws Exception { + public NetworkAgentWrapper(int transport, LinkProperties linkProperties, + NetworkCapabilities ncTemplate, Context context) throws Exception { final int type = transportToLegacyType(transport); final String typeName = ConnectivityManager.getNetworkTypeName(type); mNetworkInfo = new NetworkInfo(type, 0, typeName, "Mock"); - mNetworkCapabilities = new NetworkCapabilities(); + mNetworkCapabilities = (ncTemplate != null) ? ncTemplate : new NetworkCapabilities(); mNetworkCapabilities.addCapability(NET_CAPABILITY_NOT_SUSPENDED); mNetworkCapabilities.addTransportType(transport); switch (transport) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index c0293c157c277..810ccbe252ca0 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -606,12 +606,17 @@ public class ConnectivityServiceTest { private String mRedirectUrl; TestNetworkAgentWrapper(int transport) throws Exception { - this(transport, new LinkProperties()); + this(transport, new LinkProperties(), null); } TestNetworkAgentWrapper(int transport, LinkProperties linkProperties) throws Exception { - super(transport, linkProperties, mServiceContext); + this(transport, linkProperties, null); + } + + private TestNetworkAgentWrapper(int transport, LinkProperties linkProperties, + NetworkCapabilities ncTemplate) throws Exception { + super(transport, linkProperties, ncTemplate, mServiceContext); // Waits for the NetworkAgent to be registered, which includes the creation of the // NetworkMonitor. @@ -1006,30 +1011,26 @@ public class ConnectivityServiceTest { } } + private Set uidRangesForUid(int uid) { + final ArraySet ranges = new ArraySet<>(); + ranges.add(new UidRange(uid, uid)); + return ranges; + } + private static Looper startHandlerThreadAndReturnLooper() { final HandlerThread handlerThread = new HandlerThread("MockVpnThread"); handlerThread.start(); return handlerThread.getLooper(); } - private class MockVpn extends Vpn { - // TODO : the interactions between this mock and the mock network agent are too - // hard to get right at this moment, because it's unclear in which case which - // target needs to get a method call or both, and in what order. It's because - // MockNetworkAgent wants to manage its own NetworkCapabilities, but the Vpn - // parent class of MockVpn agent wants that responsibility. - // That being said inside the test it should be possible to make the interactions - // harder to get wrong with precise speccing, judicious comments, helper methods - // and a few sprinkled assertions. - - private boolean mConnected = false; + private class MockVpn extends Vpn implements TestableNetworkCallback.HasNetwork { // Careful ! This is different from mNetworkAgent, because MockNetworkAgent does // not inherit from NetworkAgent. private TestNetworkAgentWrapper mMockNetworkAgent; - private int mVpnType = VpnManager.TYPE_VPN_SERVICE; + private boolean mAgentRegistered = false; + private int mVpnType = VpnManager.TYPE_VPN_SERVICE; private VpnInfo mVpnInfo; - private Network[] mUnderlyingNetworks; public MockVpn(int userId) { super(startHandlerThreadAndReturnLooper(), mServiceContext, mNetworkManagementService, @@ -1037,29 +1038,23 @@ public class ConnectivityServiceTest { mConfig = new VpnConfig(); } - public void setNetworkAgent(TestNetworkAgentWrapper agent) { - agent.waitForIdle(TIMEOUT_MS); - mMockNetworkAgent = agent; - mNetworkAgent = agent.getNetworkAgent(); - mInterface = VPN_IFNAME; - mNetworkCapabilities.set(agent.getNetworkCapabilities()); - } - public void setUids(Set uids) { mNetworkCapabilities.setUids(uids); - updateCapabilities(null /* defaultNetwork */); + updateCapabilitiesInternal(null /* defaultNetwork */, true); } public void setVpnType(int vpnType) { mVpnType = vpnType; } + @Override + public Network getNetwork() { + return (mMockNetworkAgent == null) ? null : mMockNetworkAgent.getNetwork(); + } + @Override public int getNetId() { - if (mMockNetworkAgent == null) { - return NETID_UNSET; - } - return mMockNetworkAgent.getNetwork().netId; + return (mMockNetworkAgent == null) ? NETID_UNSET : mMockNetworkAgent.getNetwork().netId; } @Override @@ -1067,39 +1062,94 @@ public class ConnectivityServiceTest { return mVpnType; } - private void connect(boolean isAlwaysMetered) { - mNetworkCapabilities.set(mMockNetworkAgent.getNetworkCapabilities()); - mConnected = true; + private void registerAgent(boolean isAlwaysMetered, Set uids, LinkProperties lp) + throws Exception { + if (mAgentRegistered) throw new IllegalStateException("already registered"); + setUids(uids); mConfig.isMetered = isAlwaysMetered; + mInterface = VPN_IFNAME; + mMockNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN, lp, + mNetworkCapabilities); + mMockNetworkAgent.waitForIdle(TIMEOUT_MS); + mAgentRegistered = true; + mNetworkCapabilities.set(mMockNetworkAgent.getNetworkCapabilities()); + mNetworkAgent = mMockNetworkAgent.getNetworkAgent(); } - public void connectAsAlwaysMetered() { - connect(true /* isAlwaysMetered */); + private void registerAgent(Set uids) throws Exception { + registerAgent(false /* isAlwaysMetered */, uids, new LinkProperties()); } - public void connect() { - connect(false /* isAlwaysMetered */); + private void connect(boolean validated, boolean hasInternet, boolean isStrictMode) { + mMockNetworkAgent.connect(validated, hasInternet, isStrictMode); + } + + private void connect(boolean validated) { + mMockNetworkAgent.connect(validated); + } + + private TestNetworkAgentWrapper getAgent() { + return mMockNetworkAgent; + } + + public void establish(LinkProperties lp, int uid, Set ranges, boolean validated, + boolean hasInternet, boolean isStrictMode) throws Exception { + mNetworkCapabilities.setOwnerUid(uid); + mNetworkCapabilities.setAdministratorUids(new int[]{uid}); + registerAgent(false, ranges, lp); + connect(validated, hasInternet, isStrictMode); + waitForIdle(); + } + + public void establish(LinkProperties lp, int uid, Set ranges) throws Exception { + establish(lp, uid, ranges, true, true, false); + } + + public void establishForMyUid(LinkProperties lp) throws Exception { + final int uid = Process.myUid(); + establish(lp, uid, uidRangesForUid(uid), true, true, false); + } + + public void establishForMyUid(boolean validated, boolean hasInternet, boolean isStrictMode) + throws Exception { + final int uid = Process.myUid(); + establish(new LinkProperties(), uid, uidRangesForUid(uid), validated, hasInternet, + isStrictMode); + } + + public void establishForMyUid() throws Exception { + establishForMyUid(new LinkProperties()); + } + + public void sendLinkProperties(LinkProperties lp) { + mMockNetworkAgent.sendLinkProperties(lp); + } + + private NetworkCapabilities updateCapabilitiesInternal(Network defaultNetwork, + boolean sendToConnectivityService) { + if (!mAgentRegistered) return null; + super.updateCapabilities(defaultNetwork); + // Because super.updateCapabilities will update the capabilities of the agent but + // not the mock agent, the mock agent needs to know about them. + copyCapabilitiesToNetworkAgent(sendToConnectivityService); + return new NetworkCapabilities(mNetworkCapabilities); + } + + private void copyCapabilitiesToNetworkAgent(boolean sendToConnectivityService) { + if (null != mMockNetworkAgent) { + mMockNetworkAgent.setNetworkCapabilities(mNetworkCapabilities, + sendToConnectivityService); + } } @Override public NetworkCapabilities updateCapabilities(Network defaultNetwork) { - if (!mConnected) return null; - super.updateCapabilities(defaultNetwork); - // Because super.updateCapabilities will update the capabilities of the agent but - // not the mock agent, the mock agent needs to know about them. - copyCapabilitiesToNetworkAgent(); - return new NetworkCapabilities(mNetworkCapabilities); - } - - private void copyCapabilitiesToNetworkAgent() { - if (null != mMockNetworkAgent) { - mMockNetworkAgent.setNetworkCapabilities(mNetworkCapabilities, - false /* sendToConnectivityService */); - } + return updateCapabilitiesInternal(defaultNetwork, false); } public void disconnect() { - mConnected = false; + if (mMockNetworkAgent != null) mMockNetworkAgent.disconnect(); + mAgentRegistered = false; } @Override @@ -1305,6 +1355,9 @@ public class ConnectivityServiceTest { mEthernetNetworkAgent.disconnect(); mEthernetNetworkAgent = null; } + mMockVpn.disconnect(); + waitForIdle(); + FakeSettingsProvider.clearSettingsProvider(); mCsHandlerThread.quitSafely(); @@ -3180,20 +3233,12 @@ public class ConnectivityServiceTest { waitForIdle(); assertEquals(null, mCm.getActiveNetwork()); - final int uid = Process.myUid(); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true); - mMockVpn.connect(); - defaultNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); + mMockVpn.establishForMyUid(); + defaultNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); assertEquals(defaultNetworkCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - vpnNetworkAgent.disconnect(); - defaultNetworkCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + mMockVpn.disconnect(); + defaultNetworkCallback.expectCallback(CallbackEntry.LOST, mMockVpn); waitForIdle(); assertEquals(null, mCm.getActiveNetwork()); } @@ -4847,9 +4892,10 @@ public class ConnectivityServiceTest { final LinkProperties lp = new LinkProperties(); lp.setInterfaceName(VPN_IFNAME); - final NetworkAgentWrapper vpnNetworkAgent = establishVpnForMyUid(lp); + mMockVpn.establishForMyUid(lp); + final Network[] cellAndVpn = new Network[] { - mCellNetworkAgent.getNetwork(), vpnNetworkAgent.getNetwork()}; + mCellNetworkAgent.getNetwork(), mMockVpn.getNetwork()}; Network[] cellAndWifi = new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; @@ -4862,7 +4908,7 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent.connect(false); mWiFiNetworkAgent.sendLinkProperties(wifiLp); final Network[] wifiAndVpn = new Network[] { - mWiFiNetworkAgent.getNetwork(), vpnNetworkAgent.getNetwork()}; + mWiFiNetworkAgent.getNetwork(), mMockVpn.getNetwork()}; cellAndWifi = new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; @@ -5301,22 +5347,13 @@ public class ConnectivityServiceTest { // Note: it's sort of meaningless for a VPN app to declare a network that doesn't exist yet, // (and doing so is difficult without using reflection) but it's good to test that the code // behaves approximately correctly. - final int uid = Process.myUid(); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - + mMockVpn.establishForMyUid(false, true, false); final Network wifiNetwork = new Network(mNetIdManager.peekNextNetId()); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); mService.setUnderlyingNetworksForVpn(new Network[]{wifiNetwork}); - vpnNetworkAgent.connect(false); - mMockVpn.connect(); - callback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); - assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + callback.expectAvailableCallbacksUnvalidated(mMockVpn); + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasTransport(TRANSPORT_VPN)); - assertFalse(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + assertFalse(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasTransport(TRANSPORT_WIFI)); // Make that underlying network connect, and expect to see its capabilities immediately @@ -5333,20 +5370,20 @@ public class ConnectivityServiceTest { // 2. When a network connects, updateNetworkInfo propagates underlying network capabilities // before rematching networks. // Given that this scenario can't really happen, this is probably fine for now. - callback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + callback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); - assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasTransport(TRANSPORT_VPN)); - assertTrue(mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()) + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasTransport(TRANSPORT_WIFI)); // Disconnect the network, and expect to see the VPN capabilities change accordingly. mWiFiNetworkAgent.disconnect(); callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); - callback.expectCapabilitiesThat(vpnNetworkAgent, (nc) -> + callback.expectCapabilitiesThat(mMockVpn, (nc) -> nc.getTransportTypes().length == 1 && nc.hasTransport(TRANSPORT_VPN)); - vpnNetworkAgent.disconnect(); + mMockVpn.disconnect(); mCm.unregisterNetworkCallback(callback); } @@ -5384,42 +5421,38 @@ public class ConnectivityServiceTest { vpnNetworkCallback.assertNoCallback(); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); + final Set ranges = uidRangesForUid(uid); + mMockVpn.registerAgent(ranges); + // VPN networks do not satisfy the default request and are automatically validated // by NetworkMonitor assertFalse(NetworkMonitorUtils.isValidationRequired( - vpnNetworkAgent.getNetworkCapabilities())); - vpnNetworkAgent.setNetworkValid(false /* isStrictMode */); + mMockVpn.getAgent().getNetworkCapabilities())); + mMockVpn.getAgent().setNetworkValid(false /* isStrictMode */); - vpnNetworkAgent.connect(false); - mMockVpn.connect(); + mMockVpn.connect(false); mService.setUnderlyingNetworksForVpn(new Network[0]); - genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); + genericNetworkCallback.expectAvailableCallbacksUnvalidated(mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); wifiNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); - defaultCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); + vpnNetworkCallback.expectAvailableCallbacksUnvalidated(mMockVpn); + defaultCallback.expectAvailableCallbacksUnvalidated(mMockVpn); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - genericNetworkCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + genericNetworkCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, nc -> null == nc.getUids()); - defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, nc -> null == nc.getUids()); + defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); ranges.clear(); - vpnNetworkAgent.setUids(ranges); + mMockVpn.setUids(ranges); - genericNetworkCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + genericNetworkCallback.expectCallback(CallbackEntry.LOST, mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); wifiNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + vpnNetworkCallback.expectCallback(CallbackEntry.LOST, mMockVpn); // TODO : The default network callback should actually get a LOST call here (also see the // comment below for AVAILABLE). This is because ConnectivityService does not look at UID @@ -5427,19 +5460,18 @@ public class ConnectivityServiceTest { // can't currently update their UIDs without disconnecting, so this does not matter too // much, but that is the reason the test here has to check for an update to the // capabilities instead of the expected LOST then AVAILABLE. - defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); ranges.add(new UidRange(uid, uid)); mMockVpn.setUids(ranges); - vpnNetworkAgent.setUids(ranges); - genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent); + genericNetworkCallback.expectAvailableCallbacksValidated(mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); wifiNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent); + vpnNetworkCallback.expectAvailableCallbacksValidated(mMockVpn); // TODO : Here like above, AVAILABLE would be correct, but because this can't actually // happen outside of the test, ConnectivityService does not rematch callbacks. - defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, vpnNetworkAgent); + defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); mWiFiNetworkAgent.disconnect(); @@ -5449,13 +5481,13 @@ public class ConnectivityServiceTest { vpnNetworkCallback.assertNoCallback(); defaultCallback.assertNoCallback(); - vpnNetworkAgent.disconnect(); + mMockVpn.disconnect(); - genericNetworkCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + genericNetworkCallback.expectCallback(CallbackEntry.LOST, mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); wifiNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); - defaultCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + vpnNetworkCallback.expectCallback(CallbackEntry.LOST, mMockVpn); + defaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); assertEquals(null, mCm.getActiveNetwork()); mCm.unregisterNetworkCallback(genericNetworkCallback); @@ -5477,20 +5509,13 @@ public class ConnectivityServiceTest { defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */, + mMockVpn.establishForMyUid(true /* validated */, false /* hasInternet */, false /* isStrictMode */); - mMockVpn.connect(); defaultCallback.assertNoCallback(); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - vpnNetworkAgent.disconnect(); + mMockVpn.disconnect(); defaultCallback.assertNoCallback(); mCm.unregisterNetworkCallback(defaultCallback); @@ -5509,21 +5534,14 @@ public class ConnectivityServiceTest { defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, true /* hasInternet */, + mMockVpn.establishForMyUid(true /* validated */, true /* hasInternet */, false /* isStrictMode */); - mMockVpn.connect(); - defaultCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); + defaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - vpnNetworkAgent.disconnect(); - defaultCallback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + mMockVpn.disconnect(); + defaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); defaultCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent); mCm.unregisterNetworkCallback(defaultCallback); @@ -5541,44 +5559,36 @@ public class ConnectivityServiceTest { callback.assertNoCallback(); // Bring up a VPN that has the INTERNET capability, initially unvalidated. - final int uid = Process.myUid(); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */, + mMockVpn.establishForMyUid(false /* validated */, true /* hasInternet */, false /* isStrictMode */); - mMockVpn.connect(); // Even though the VPN is unvalidated, it becomes the default network for our app. - callback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); + callback.expectAvailableCallbacksUnvalidated(mMockVpn); callback.assertNoCallback(); - assertTrue(vpnNetworkAgent.getScore() > mEthernetNetworkAgent.getScore()); - assertEquals(ConnectivityConstants.VPN_DEFAULT_SCORE, vpnNetworkAgent.getScore()); - assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertTrue(mMockVpn.getAgent().getScore() > mEthernetNetworkAgent.getScore()); + assertEquals(ConnectivityConstants.VPN_DEFAULT_SCORE, mMockVpn.getAgent().getScore()); + assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); - NetworkCapabilities nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + NetworkCapabilities nc = mCm.getNetworkCapabilities(mMockVpn.getNetwork()); assertFalse(nc.hasCapability(NET_CAPABILITY_VALIDATED)); assertTrue(nc.hasCapability(NET_CAPABILITY_INTERNET)); assertFalse(NetworkMonitorUtils.isValidationRequired( - vpnNetworkAgent.getNetworkCapabilities())); + mMockVpn.getAgent().getNetworkCapabilities())); assertTrue(NetworkMonitorUtils.isPrivateDnsValidationRequired( - vpnNetworkAgent.getNetworkCapabilities())); + mMockVpn.getAgent().getNetworkCapabilities())); // Pretend that the VPN network validates. - vpnNetworkAgent.setNetworkValid(false /* isStrictMode */); - vpnNetworkAgent.mNetworkMonitor.forceReevaluation(Process.myUid()); + mMockVpn.getAgent().setNetworkValid(false /* isStrictMode */); + mMockVpn.getAgent().mNetworkMonitor.forceReevaluation(Process.myUid()); // Expect to see the validated capability, but no other changes, because the VPN is already // the default network for the app. - callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, vpnNetworkAgent); + callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mMockVpn); callback.assertNoCallback(); - vpnNetworkAgent.disconnect(); - callback.expectCallback(CallbackEntry.LOST, vpnNetworkAgent); + mMockVpn.disconnect(); + callback.expectCallback(CallbackEntry.LOST, mMockVpn); callback.expectAvailableCallbacksValidated(mEthernetNetworkAgent); } @@ -5600,21 +5610,15 @@ public class ConnectivityServiceTest { mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED); mCellNetworkAgent.connect(true); - final TestNetworkAgentWrapper vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */, + mMockVpn.establishForMyUid(true /* validated */, false /* hasInternet */, false /* isStrictMode */); - vpnNetworkCallback.expectAvailableCallbacks(vpnNetworkAgent.getNetwork(), + vpnNetworkCallback.expectAvailableCallbacks(mMockVpn.getNetwork(), false /* suspended */, false /* validated */, false /* blocked */, TIMEOUT_MS); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent.getNetwork(), TIMEOUT_MS, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn.getNetwork(), TIMEOUT_MS, nc -> nc.hasCapability(NET_CAPABILITY_VALIDATED)); - final NetworkCapabilities nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + final NetworkCapabilities nc = mCm.getNetworkCapabilities(mMockVpn.getNetwork()); assertTrue(nc.hasTransport(TRANSPORT_VPN)); assertTrue(nc.hasTransport(TRANSPORT_CELLULAR)); assertFalse(nc.hasTransport(TRANSPORT_WIFI)); @@ -5636,18 +5640,11 @@ public class ConnectivityServiceTest { mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback); vpnNetworkCallback.assertNoCallback(); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */, + mMockVpn.establishForMyUid(true /* validated */, false /* hasInternet */, false /* isStrictMode */); - vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); - nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + vpnNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); + nc = mCm.getNetworkCapabilities(mMockVpn.getNetwork()); assertTrue(nc.hasTransport(TRANSPORT_VPN)); assertFalse(nc.hasTransport(TRANSPORT_CELLULAR)); assertFalse(nc.hasTransport(TRANSPORT_WIFI)); @@ -5664,7 +5661,7 @@ public class ConnectivityServiceTest { mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5678,7 +5675,7 @@ public class ConnectivityServiceTest { mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5688,7 +5685,7 @@ public class ConnectivityServiceTest { mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5696,27 +5693,27 @@ public class ConnectivityServiceTest { // Remove NOT_SUSPENDED from the only network and observe VPN is now suspended. mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_SUSPENDED); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && !caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); - vpnNetworkCallback.expectCallback(CallbackEntry.SUSPENDED, vpnNetworkAgent); + vpnNetworkCallback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn); // Add NOT_SUSPENDED again and observe VPN is no longer suspended. mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); - vpnNetworkCallback.expectCallback(CallbackEntry.RESUMED, vpnNetworkAgent); + vpnNetworkCallback.expectCallback(CallbackEntry.RESUMED, mMockVpn); // Use Wifi but not cell. Note the VPN is now unmetered and not suspended. mService.setUnderlyingNetworksForVpn( new Network[] { mWiFiNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5726,7 +5723,7 @@ public class ConnectivityServiceTest { mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5739,7 +5736,7 @@ public class ConnectivityServiceTest { // Stop using WiFi. The VPN is suspended again. mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5753,7 +5750,7 @@ public class ConnectivityServiceTest { mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork() }); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) @@ -5764,14 +5761,14 @@ public class ConnectivityServiceTest { // Disconnect cell. Receive update without even removing the dead network from the // underlying networks – it's dead anyway. Not metered any more. mCellNetworkAgent.disconnect(); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && caps.hasCapability(NET_CAPABILITY_NOT_METERED)); // Disconnect wifi too. No underlying networks means this is now metered. mWiFiNetworkAgent.disconnect(); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)); @@ -5792,18 +5789,11 @@ public class ConnectivityServiceTest { mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback); vpnNetworkCallback.assertNoCallback(); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */, + mMockVpn.establishForMyUid(true /* validated */, false /* hasInternet */, false /* isStrictMode */); - vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); - nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + vpnNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); + nc = mCm.getNetworkCapabilities(mMockVpn.getNetwork()); assertTrue(nc.hasTransport(TRANSPORT_VPN)); assertFalse(nc.hasTransport(TRANSPORT_CELLULAR)); assertFalse(nc.hasTransport(TRANSPORT_WIFI)); @@ -5815,7 +5805,7 @@ public class ConnectivityServiceTest { mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); mCellNetworkAgent.connect(true); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)); @@ -5825,7 +5815,7 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); mWiFiNetworkAgent.connect(true); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && caps.hasCapability(NET_CAPABILITY_NOT_METERED)); @@ -5837,7 +5827,7 @@ public class ConnectivityServiceTest { // Disconnect wifi too. Now we have no default network. mWiFiNetworkAgent.disconnect(); - vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent, + vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)); @@ -5880,18 +5870,10 @@ public class ConnectivityServiceTest { assertTrue(mCm.isActiveNetworkMetered()); // Connect VPN network. By default it is using current default network (Cell). - TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - final int uid = Process.myUid(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true); - mMockVpn.connect(); - waitForIdle(); + mMockVpn.establishForMyUid(); + // Ensure VPN is now the active network. - assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); // Expect VPN to be metered. assertTrue(mCm.isActiveNetworkMetered()); @@ -5902,7 +5884,7 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent.connect(true); waitForIdle(); // VPN should still be the active network. - assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); // Expect VPN to be unmetered as it should now be using WiFi (new default). assertFalse(mCm.isActiveNetworkMetered()); @@ -5920,7 +5902,6 @@ public class ConnectivityServiceTest { // VPN without any underlying networks is treated as metered. assertTrue(mCm.isActiveNetworkMetered()); - vpnNetworkAgent.disconnect(); mMockVpn.disconnect(); } @@ -5941,18 +5922,10 @@ public class ConnectivityServiceTest { assertFalse(mCm.isActiveNetworkMetered()); // Connect VPN network. - TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - final int uid = Process.myUid(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true); - mMockVpn.connect(); - waitForIdle(); + mMockVpn.establishForMyUid(); + // Ensure VPN is now the active network. - assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); // VPN is using Cell mService.setUnderlyingNetworksForVpn( new Network[] { mCellNetworkAgent.getNetwork() }); @@ -5992,7 +5965,6 @@ public class ConnectivityServiceTest { // VPN without underlying networks is treated as metered. assertTrue(mCm.isActiveNetworkMetered()); - vpnNetworkAgent.disconnect(); mMockVpn.disconnect(); } @@ -6007,17 +5979,11 @@ public class ConnectivityServiceTest { assertFalse(mCm.isActiveNetworkMetered()); // Connect VPN network. - TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - final int uid = Process.myUid(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.setUids(ranges); - vpnNetworkAgent.connect(true); - mMockVpn.connectAsAlwaysMetered(); + mMockVpn.registerAgent(true /* isAlwaysMetered */, uidRangesForUid(Process.myUid()), + new LinkProperties()); + mMockVpn.connect(true); waitForIdle(); - assertEquals(vpnNetworkAgent.getNetwork(), mCm.getActiveNetwork()); + assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); // VPN is tracking current platform default (WiFi). mService.setUnderlyingNetworksForVpn(null); @@ -6041,7 +6007,7 @@ public class ConnectivityServiceTest { assertTrue(mCm.isActiveNetworkMetered()); - vpnNetworkAgent.disconnect(); + mMockVpn.disconnect(); } @Test @@ -6766,34 +6732,21 @@ public class ConnectivityServiceTest { waitForIdle(); assertNull(mService.getProxyForNetwork(null)); - // Set up a VPN network with a proxy - final int uid = Process.myUid(); - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - mMockVpn.setUids(ranges); + // Connect a VPN network with a proxy. LinkProperties testLinkProperties = new LinkProperties(); testLinkProperties.setHttpProxy(testProxyInfo); - vpnNetworkAgent.sendLinkProperties(testLinkProperties); - waitForIdle(); - - // Connect to VPN with proxy - mMockVpn.setNetworkAgent(vpnNetworkAgent); - vpnNetworkAgent.connect(true); - mMockVpn.connect(); - waitForIdle(); + mMockVpn.establishForMyUid(testLinkProperties); // Test that the VPN network returns a proxy, and the WiFi does not. - assertEquals(testProxyInfo, mService.getProxyForNetwork(vpnNetworkAgent.getNetwork())); + assertEquals(testProxyInfo, mService.getProxyForNetwork(mMockVpn.getNetwork())); assertEquals(testProxyInfo, mService.getProxyForNetwork(null)); assertNull(mService.getProxyForNetwork(mWiFiNetworkAgent.getNetwork())); // Test that the VPN network returns no proxy when it is set to null. testLinkProperties.setHttpProxy(null); - vpnNetworkAgent.sendLinkProperties(testLinkProperties); + mMockVpn.sendLinkProperties(testLinkProperties); waitForIdle(); - assertNull(mService.getProxyForNetwork(vpnNetworkAgent.getNetwork())); + assertNull(mService.getProxyForNetwork(mMockVpn.getNetwork())); assertNull(mService.getProxyForNetwork(null)); // Set WiFi proxy and check that the vpn proxy is still null. @@ -6804,7 +6757,7 @@ public class ConnectivityServiceTest { // Disconnect from VPN and check that the active network, which is now the WiFi, has the // correct proxy setting. - vpnNetworkAgent.disconnect(); + mMockVpn.disconnect(); waitForIdle(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertEquals(testProxyInfo, mService.getProxyForNetwork(mWiFiNetworkAgent.getNetwork())); @@ -6819,7 +6772,7 @@ public class ConnectivityServiceTest { lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), RTN_UNREACHABLE)); // The uid range needs to cover the test app so the network is visible to it. final Set vpnRange = Collections.singleton(UidRange.createForUser(VPN_USER)); - final TestNetworkAgentWrapper vpnNetworkAgent = establishVpn(lp, VPN_UID, vpnRange); + mMockVpn.establish(lp, VPN_UID, vpnRange); // Connected VPN should have interface rules set up. There are two expected invocations, // one during VPN uid update, one during VPN LinkProperties update @@ -6829,7 +6782,7 @@ public class ConnectivityServiceTest { assertContainsExactly(uidCaptor.getAllValues().get(1), APP1_UID, APP2_UID); assertTrue(mService.mPermissionMonitor.getVpnUidRanges("tun0").equals(vpnRange)); - vpnNetworkAgent.disconnect(); + mMockVpn.disconnect(); waitForIdle(); // Disconnected VPN should have interface rules removed @@ -6846,8 +6799,7 @@ public class ConnectivityServiceTest { lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), null)); // The uid range needs to cover the test app so the network is visible to it. final Set vpnRange = Collections.singleton(UidRange.createForUser(VPN_USER)); - final TestNetworkAgentWrapper vpnNetworkAgent = establishVpn( - lp, Process.SYSTEM_UID, vpnRange); + mMockVpn.establish(lp, Process.SYSTEM_UID, vpnRange); // Legacy VPN should not have interface rules set up verify(mMockNetd, never()).firewallAddUidInterfaceRules(any(), any()); @@ -6862,8 +6814,7 @@ public class ConnectivityServiceTest { lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), RTN_UNREACHABLE)); // The uid range needs to cover the test app so the network is visible to it. final Set vpnRange = Collections.singleton(UidRange.createForUser(VPN_USER)); - final TestNetworkAgentWrapper vpnNetworkAgent = establishVpn( - lp, Process.SYSTEM_UID, vpnRange); + mMockVpn.establish(lp, Process.SYSTEM_UID, vpnRange); // IPv6 unreachable route should not be misinterpreted as a default route verify(mMockNetd, never()).firewallAddUidInterfaceRules(any(), any()); @@ -6877,7 +6828,7 @@ public class ConnectivityServiceTest { lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), null)); // The uid range needs to cover the test app so the network is visible to it. final Set vpnRange = Collections.singleton(UidRange.createForUser(VPN_USER)); - final TestNetworkAgentWrapper vpnNetworkAgent = establishVpn(lp, VPN_UID, vpnRange); + mMockVpn.establish(lp, VPN_UID, vpnRange); // Connected VPN should have interface rules set up. There are two expected invocations, // one during VPN uid update, one during VPN LinkProperties update @@ -6889,7 +6840,7 @@ public class ConnectivityServiceTest { reset(mMockNetd); InOrder inOrder = inOrder(mMockNetd); lp.setInterfaceName("tun1"); - vpnNetworkAgent.sendLinkProperties(lp); + mMockVpn.sendLinkProperties(lp); waitForIdle(); // VPN handover (switch to a new interface) should result in rules being updated (old rules // removed first, then new rules added) @@ -6902,7 +6853,7 @@ public class ConnectivityServiceTest { lp = new LinkProperties(); lp.setInterfaceName("tun1"); lp.addRoute(new RouteInfo(new IpPrefix("192.0.2.0/24"), null, "tun1")); - vpnNetworkAgent.sendLinkProperties(lp); + mMockVpn.sendLinkProperties(lp); waitForIdle(); // VPN not routing everything should no longer have interface filtering rules verify(mMockNetd).firewallRemoveUidInterfaceRules(uidCaptor.capture()); @@ -6913,7 +6864,7 @@ public class ConnectivityServiceTest { lp.setInterfaceName("tun1"); lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), RTN_UNREACHABLE)); lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), null)); - vpnNetworkAgent.sendLinkProperties(lp); + mMockVpn.sendLinkProperties(lp); waitForIdle(); // Back to routing all IPv6 traffic should have filtering rules verify(mMockNetd).firewallAddUidInterfaceRules(eq("tun1"), uidCaptor.capture()); @@ -6928,8 +6879,7 @@ public class ConnectivityServiceTest { lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), null)); // The uid range needs to cover the test app so the network is visible to it. final UidRange vpnRange = UidRange.createForUser(VPN_USER); - final TestNetworkAgentWrapper vpnNetworkAgent = establishVpn(lp, VPN_UID, - Collections.singleton(vpnRange)); + mMockVpn.establish(lp, VPN_UID, Collections.singleton(vpnRange)); reset(mMockNetd); InOrder inOrder = inOrder(mMockNetd); @@ -6938,7 +6888,7 @@ public class ConnectivityServiceTest { final Set newRanges = new HashSet<>(Arrays.asList( new UidRange(vpnRange.start, APP1_UID - 1), new UidRange(APP1_UID + 1, vpnRange.stop))); - vpnNetworkAgent.setUids(newRanges); + mMockVpn.setUids(newRanges); waitForIdle(); ArgumentCaptor uidCaptor = ArgumentCaptor.forClass(int[].class); @@ -7079,7 +7029,7 @@ public class ConnectivityServiceTest { private void setupConnectionOwnerUid(int vpnOwnerUid, @VpnManager.VpnType int vpnType) throws Exception { final Set vpnRange = Collections.singleton(UidRange.createForUser(VPN_USER)); - establishVpn(new LinkProperties(), vpnOwnerUid, vpnRange); + mMockVpn.establish(new LinkProperties(), vpnOwnerUid, vpnRange); mMockVpn.setVpnType(vpnType); final VpnInfo vpnInfo = new VpnInfo(); @@ -7160,27 +7110,6 @@ public class ConnectivityServiceTest { mService.getConnectionOwnerUid(getTestConnectionInfo()); } - private TestNetworkAgentWrapper establishVpn( - LinkProperties lp, int ownerUid, Set vpnRange) throws Exception { - final TestNetworkAgentWrapper - vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN, lp); - vpnNetworkAgent.getNetworkCapabilities().setOwnerUid(ownerUid); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); - mMockVpn.setUids(vpnRange); - vpnNetworkAgent.connect(true); - waitForIdle(); - return vpnNetworkAgent; - } - - private TestNetworkAgentWrapper establishVpnForMyUid(LinkProperties lp) - throws Exception { - final int uid = Process.myUid(); - final ArraySet ranges = new ArraySet<>(); - ranges.add(new UidRange(uid, uid)); - return establishVpn(lp, uid, ranges); - } - private static PackageInfo buildPackageInfo(boolean hasSystemPermission, int uid) { final PackageInfo packageInfo = new PackageInfo(); if (hasSystemPermission) { @@ -7360,7 +7289,6 @@ public class ConnectivityServiceTest { setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); - mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); // setUp() calls mockVpn() which adds a VPN with the Test Runner's uid. Configure it to be // active @@ -7369,9 +7297,11 @@ public class ConnectivityServiceTest { info.vpnIface = VPN_IFNAME; mMockVpn.setVpnInfo(info); - final TestNetworkAgentWrapper vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN); - mMockVpn.setNetworkAgent(vpnNetworkAgent); - mMockVpn.connect(); + mMockVpn.establishForMyUid(); + waitForIdle(); + + mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); + assertTrue(mService.setUnderlyingNetworksForVpn(new Network[] {network})); assertTrue( @@ -7401,8 +7331,6 @@ public class ConnectivityServiceTest { Manifest.permission.ACCESS_FINE_LOCATION); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); - // Disconnect mock vpn so the uid check on NetworkAgentInfo is tested - mMockVpn.disconnect(); assertTrue( "NetworkCapabilities administrator uid permission not applied", mService.checkConnectivityDiagnosticsPermissions( From 287e315cddbbb311d781603cb386673e74abce7a Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 19 Nov 2020 23:20:55 +0900 Subject: [PATCH 16/24] Make testVpnNetworkActive more deterministic. This test is a bit brittle because it sets the underlying networks while the VPN is undergoing validation by NetworkMonitor. The test does attempt to disable validation, but that's not actually possible - the only thing that's possible is to tell NetworkMonitor to validate immediately without sending any probes. So the underlying network change races with the validation. I'm not sure why the test isn't flaky. It might be because both the network change and the validation result in a capabilities change, and the test expects "a capabilities change" without expressing what change that should be. Make this a bit more predictable by ensuring that the network validates before the underlying networks are set. This is useful because an upcoming CL will change the way underlying network capabilities are propagated. With this test CL, both the old and the new code pass. Bug: 172870110 Test: test-only change Change-Id: I319858228e8d097c0b60a107029f296385f91269 Merged-In: I319858228e8d097c0b60a107029f296385f91269 (cherry picked from commit 0b53357c1a74300319a61890201e56916864309c) --- .../android/server/ConnectivityServiceTest.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 810ccbe252ca0..bb39af9a1b9e9 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5423,6 +5423,7 @@ public class ConnectivityServiceTest { final Set ranges = uidRangesForUid(uid); mMockVpn.registerAgent(ranges); + mService.setUnderlyingNetworksForVpn(new Network[0]); // VPN networks do not satisfy the default request and are automatically validated // by NetworkMonitor @@ -5431,19 +5432,12 @@ public class ConnectivityServiceTest { mMockVpn.getAgent().setNetworkValid(false /* isStrictMode */); mMockVpn.connect(false); - mService.setUnderlyingNetworksForVpn(new Network[0]); - genericNetworkCallback.expectAvailableCallbacksUnvalidated(mMockVpn); + genericNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); genericNotVpnNetworkCallback.assertNoCallback(); wifiNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectAvailableCallbacksUnvalidated(mMockVpn); - defaultCallback.expectAvailableCallbacksUnvalidated(mMockVpn); - assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - - genericNetworkCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); - genericNotVpnNetworkCallback.assertNoCallback(); - vpnNetworkCallback.expectCapabilitiesThat(mMockVpn, nc -> null == nc.getUids()); - defaultCallback.expectCallback(CallbackEntry.NETWORK_CAPS_UPDATED, mMockVpn); + vpnNetworkCallback.expectAvailableThenValidatedCallbacks(mMockVpn); + defaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); ranges.clear(); From 3007352939aeb816f9447ec9d93433f6d27a969d Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 25 Nov 2020 22:59:08 +0900 Subject: [PATCH 17/24] Test passing an underlying network array with null network in it. Current code treats these nulls as if they weren't there. Bug: 172870110 Test: test-only change Change-Id: Id4632e1b004c09910b4b7613f7233d2c19e2f0ac Merged-In: Id4632e1b004c09910b4b7613f7233d2c19e2f0ac (cherry picked from commit eccd26239024ca2a401e354fb16b855fa72f9923) --- .../server/ConnectivityServiceTest.java | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index bb39af9a1b9e9..9945853c5a6f9 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -4896,8 +4896,6 @@ public class ConnectivityServiceTest { final Network[] cellAndVpn = new Network[] { mCellNetworkAgent.getNetwork(), mMockVpn.getNetwork()}; - Network[] cellAndWifi = new Network[] { - mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; // A VPN with default (null) underlying networks sets the underlying network's interfaces... expectForceUpdateIfaces(cellAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, @@ -4907,10 +4905,13 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(false); mWiFiNetworkAgent.sendLinkProperties(wifiLp); + final Network[] onlyNull = new Network[]{null}; final Network[] wifiAndVpn = new Network[] { mWiFiNetworkAgent.getNetwork(), mMockVpn.getNetwork()}; - cellAndWifi = new Network[] { + final Network[] cellAndWifi = new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork()}; + final Network[] cellNullAndWifi = new Network[] { + mCellNetworkAgent.getNetwork(), null, mWiFiNetworkAgent.getNetwork()}; waitForIdle(); assertEquals(wifiLp, mService.getActiveLinkProperties()); @@ -4936,6 +4937,13 @@ public class ConnectivityServiceTest { new String[]{MOBILE_IFNAME, WIFI_IFNAME}); reset(mStatsService); + // Null underlying networks are ignored. + mService.setUnderlyingNetworksForVpn(cellNullAndWifi); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, MOBILE_IFNAME, Process.myUid(), VPN_IFNAME, + new String[]{MOBILE_IFNAME, WIFI_IFNAME}); + reset(mStatsService); + // If an underlying network disconnects, that interface should no longer be underlying. // This doesn't actually work because disconnectAndDestroyNetwork only notifies // NetworkStatsService before the underlying network is actually removed. So the underlying @@ -4970,6 +4978,7 @@ public class ConnectivityServiceTest { argThat(vpnInfos -> vpnInfos[0].underlyingIfaces.length == 1 && WIFI_IFNAME.equals(vpnInfos[0].underlyingIfaces[0]))); mEthernetNetworkAgent.disconnect(); + waitForIdle(); reset(mStatsService); // When a VPN declares no underlying networks (i.e., no connectivity), getAllVpnInfo @@ -4982,6 +4991,18 @@ public class ConnectivityServiceTest { waitForIdle(); expectForceUpdateIfaces(wifiAndVpn, null); reset(mStatsService); + + // Specifying only a null underlying network is the same as no networks. + mService.setUnderlyingNetworksForVpn(onlyNull); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, null); + reset(mStatsService); + + // Specifying networks that are all disconnected is the same as specifying no networks. + mService.setUnderlyingNetworksForVpn(onlyCell); + waitForIdle(); + expectForceUpdateIfaces(wifiAndVpn, null); + reset(mStatsService); } @Test From 1d1ae5e7f44d1c9e370099023877c5b8807c1a31 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 6 Jan 2021 01:36:07 +0900 Subject: [PATCH 18/24] Improve testing of CONNECTIVITY_ACTION broadcasts. We currently test CONNECTIVITY_ACTION broadcasts by directly registering BroadcastReceivers with BroadcastInterceptingContext, and making the receivers unregister themselves when all the broadcasts they expect have been received. This works for current test cases, but does not work if anything registers another receiver for CONNECTIVITY_ACTION. In that case, when we unregister the receiver in the receiver's onReceive method, BroadcastInterceptingContext will throw a ConcurrentModificationException because the list of receivers is being modified during iteration. Fix this by adding an ExpectedBroadcast class that stores the receiver and unregisters the receiver only when the test checks that the broadcast was received, which happens after the receiver runs. This is easier to use and also guarantees that the receiver is unregistered even if the test is expecting that the broadcast is never fired. Accordingly, remove mRegisteredReceivers and the code that uses it; it's no longer necessary now that ExpectedBroadcast always unregisters its receivers. Also add a convenience expectConnectivityAction method to expect a CONNECTIVITY_ACTION broadcast with specific contents. This makes the test easier to read and more detailed. Convert some existing tests to this method. While I'm at it, fix a test that was using "mCellNetworkAgent" to represent a wifi network. R backport notes: added import for NetworkInfo.DetailedState. That was added in aosp/1527378, which is impractical to backport. Bug: 172870110 Test: test-only change Change-Id: Ibada8b4215625e1016d9fd170526206920af76f5 Merged-In: Ibada8b4215625e1016d9fd170526206920af76f5 (cherry picked from commit d39cfb8de7ce7a789afd2c9e8ac3ca18fb8186ba) --- .../server/ConnectivityServiceTest.java | 335 ++++++++++-------- 1 file changed, 180 insertions(+), 155 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 9945853c5a6f9..6cbcb6784ddd6 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -170,6 +170,7 @@ import android.net.NetworkAgentConfig; import android.net.NetworkCapabilities; import android.net.NetworkFactory; import android.net.NetworkInfo; +import android.net.NetworkInfo.DetailedState; import android.net.NetworkRequest; import android.net.NetworkSpecifier; import android.net.NetworkStack; @@ -274,13 +275,16 @@ import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Predicate; import java.util.function.Supplier; @@ -384,9 +388,6 @@ public class ConnectivityServiceTest { private class MockContext extends BroadcastInterceptingContext { private final MockContentResolver mContentResolver; - // Contains all registered receivers since this object was created. Useful to clear - // them when needed, as BroadcastInterceptingContext does not provide this facility. - private final List mRegisteredReceivers = new ArrayList<>(); @Spy private Resources mResources; private final LinkedBlockingQueue mStartedActivities = new LinkedBlockingQueue<>(); @@ -504,19 +505,6 @@ public class ConnectivityServiceTest { public void setPermission(String permission, Integer granted) { mMockedPermissions.put(permission, granted); } - - @Override - public Intent registerReceiver(BroadcastReceiver receiver, IntentFilter filter) { - mRegisteredReceivers.add(receiver); - return super.registerReceiver(receiver, filter); - } - - public void clearRegisteredReceivers() { - // super.unregisterReceiver is a no-op for receivers that are not registered (because - // they haven't been registered or because they have already been unregistered). - // For the same reason, don't bother clearing mRegisteredReceivers. - for (final BroadcastReceiver rcv : mRegisteredReceivers) unregisterReceiver(rcv); - } } private void waitForIdle() { @@ -545,10 +533,10 @@ public class ConnectivityServiceTest { } // Bring up a network that we can use to send messages to ConnectivityService. - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.CONNECTED); mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); Network n = mWiFiNetworkAgent.getNetwork(); assertNotNull(n); @@ -565,10 +553,10 @@ public class ConnectivityServiceTest { @Ignore public void verifyThatNotWaitingForIdleCausesRaceConditions() throws Exception { // Bring up a network that we can use to send messages to ConnectivityService. - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.CONNECTED); mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); Network n = mWiFiNetworkAgent.getNetwork(); assertNotNull(n); @@ -1422,29 +1410,79 @@ public class ConnectivityServiceTest { } /** - * Return a ConditionVariable that opens when {@code count} numbers of CONNECTIVITY_ACTION - * broadcasts are received. + * Class to simplify expecting broadcasts using BroadcastInterceptingContext. + * Ensures that the receiver is unregistered after the expected broadcast is received. This + * cannot be done in the BroadcastReceiver itself because BroadcastInterceptingContext runs + * the receivers' receive method while iterating over the list of receivers, and unregistering + * the receiver during iteration throws ConcurrentModificationException. */ - private ConditionVariable registerConnectivityBroadcast(final int count) { + private class ExpectedBroadcast extends CompletableFuture { + private final BroadcastReceiver mReceiver; + + ExpectedBroadcast(BroadcastReceiver receiver) { + mReceiver = receiver; + } + + public Intent expectBroadcast(int timeoutMs) throws Exception { + try { + return get(timeoutMs, TimeUnit.MILLISECONDS); + } catch (TimeoutException e) { + fail("Expected broadcast not received after " + timeoutMs + " ms"); + return null; + } finally { + mServiceContext.unregisterReceiver(mReceiver); + } + } + + public Intent expectBroadcast() throws Exception { + return expectBroadcast(TIMEOUT_MS); + } + + public void expectNoBroadcast(int timeoutMs) throws Exception { + waitForIdle(); + try { + final Intent intent = get(timeoutMs, TimeUnit.MILLISECONDS); + fail("Unexpected broadcast: " + intent.getAction()); + } catch (TimeoutException expected) { + } finally { + mServiceContext.unregisterReceiver(mReceiver); + } + } + } + + /** Expects that {@code count} CONNECTIVITY_ACTION broadcasts are received. */ + private ExpectedBroadcast registerConnectivityBroadcast(final int count) { return registerConnectivityBroadcastThat(count, intent -> true); } - private ConditionVariable registerConnectivityBroadcastThat(final int count, + private ExpectedBroadcast registerConnectivityBroadcastThat(final int count, @NonNull final Predicate filter) { - final ConditionVariable cv = new ConditionVariable(); final IntentFilter intentFilter = new IntentFilter(CONNECTIVITY_ACTION); + // AtomicReference allows receiver to access expected even though it is constructed later. + final AtomicReference expectedRef = new AtomicReference<>(); final BroadcastReceiver receiver = new BroadcastReceiver() { - private int remaining = count; - public void onReceive(Context context, Intent intent) { - if (!filter.test(intent)) return; - if (--remaining == 0) { - cv.open(); - mServiceContext.unregisterReceiver(this); - } - } - }; + private int mRemaining = count; + public void onReceive(Context context, Intent intent) { + final int type = intent.getIntExtra(EXTRA_NETWORK_TYPE, -1); + final NetworkInfo ni = intent.getParcelableExtra(EXTRA_NETWORK_INFO); + Log.d(TAG, "Received CONNECTIVITY_ACTION type=" + type + " ni=" + ni); + if (!filter.test(intent)) return; + if (--mRemaining == 0) { + expectedRef.get().complete(intent); + } + } + }; + final ExpectedBroadcast expected = new ExpectedBroadcast(receiver); + expectedRef.set(expected); mServiceContext.registerReceiver(receiver, intentFilter); - return cv; + return expected; + } + + private ExpectedBroadcast expectConnectivityAction(int type, NetworkInfo.DetailedState state) { + return registerConnectivityBroadcastThat(1, intent -> + type == intent.getIntExtra(EXTRA_NETWORK_TYPE, -1) && state.equals( + ((NetworkInfo) intent.getParcelableExtra(EXTRA_NETWORK_INFO)) + .getDetailedState())); } @Test @@ -1468,10 +1506,9 @@ public class ConnectivityServiceTest { // Connect the cell agent and wait for the connected broadcast. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); mCellNetworkAgent.addCapability(NET_CAPABILITY_SUPL); - final ConditionVariable cv1 = registerConnectivityBroadcastThat(1, - intent -> intent.getIntExtra(EXTRA_NETWORK_TYPE, -1) == TYPE_MOBILE); + ExpectedBroadcast b = expectConnectivityAction(TYPE_MOBILE, DetailedState.CONNECTED); mCellNetworkAgent.connect(true); - waitFor(cv1); + b.expectBroadcast(); // Build legacy request for SUPL. final NetworkCapabilities legacyCaps = new NetworkCapabilities(); @@ -1481,27 +1518,17 @@ public class ConnectivityServiceTest { ConnectivityManager.REQUEST_ID_UNSET, NetworkRequest.Type.REQUEST); // File request, withdraw it and make sure no broadcast is sent - final ConditionVariable cv2 = registerConnectivityBroadcast(1); + b = registerConnectivityBroadcast(1); final TestNetworkCallback callback = new TestNetworkCallback(); mCm.requestNetwork(legacyRequest, callback); callback.expectCallback(CallbackEntry.AVAILABLE, mCellNetworkAgent); mCm.unregisterNetworkCallback(callback); - assertFalse(cv2.block(800)); // 800ms long enough to at least flake if this is sent - // As the broadcast did not fire, the receiver was not unregistered. Do this now. - mServiceContext.clearRegisteredReceivers(); + b.expectNoBroadcast(800); // 800ms long enough to at least flake if this is sent - // Disconnect the network and expect mobile disconnected broadcast. Use a small hack to - // check that has been sent. - final AtomicBoolean vanillaAction = new AtomicBoolean(false); - final ConditionVariable cv3 = registerConnectivityBroadcastThat(1, intent -> { - if (intent.getAction().equals(CONNECTIVITY_ACTION)) { - vanillaAction.set(true); - } - return !((NetworkInfo) intent.getExtra(EXTRA_NETWORK_INFO, -1)).isConnected(); - }); + // Disconnect the network and expect mobile disconnected broadcast. + b = expectConnectivityAction(TYPE_MOBILE, DetailedState.DISCONNECTED); mCellNetworkAgent.disconnect(); - waitFor(cv3); - assertTrue(vanillaAction.get()); + b.expectBroadcast(); } @Test @@ -1512,9 +1539,9 @@ public class ConnectivityServiceTest { assertNull(mCm.getActiveNetworkInfo()); assertNull(mCm.getActiveNetwork()); // Test bringing up validated cellular. - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = expectConnectivityAction(TYPE_MOBILE, DetailedState.CONNECTED); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); assertLength(2, mCm.getAllNetworks()); assertTrue(mCm.getAllNetworks()[0].equals(mCm.getActiveNetwork()) || @@ -1522,9 +1549,9 @@ public class ConnectivityServiceTest { assertTrue(mCm.getAllNetworks()[0].equals(mWiFiNetworkAgent.getNetwork()) || mCm.getAllNetworks()[1].equals(mWiFiNetworkAgent.getNetwork())); // Test bringing up validated WiFi. - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); assertLength(2, mCm.getAllNetworks()); assertTrue(mCm.getAllNetworks()[0].equals(mCm.getActiveNetwork()) || @@ -1539,9 +1566,9 @@ public class ConnectivityServiceTest { assertLength(1, mCm.getAllNetworks()); assertEquals(mCm.getAllNetworks()[0], mCm.getActiveNetwork()); // Test WiFi disconnect. - cv = registerConnectivityBroadcast(1); + b = registerConnectivityBroadcast(1); mWiFiNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verifyNoNetwork(); } @@ -1549,9 +1576,9 @@ public class ConnectivityServiceTest { public void testValidatedCellularOutscoresUnvalidatedWiFi() throws Exception { // Test bringing up unvalidated WiFi mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mWiFiNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Test bringing up unvalidated cellular mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); @@ -1564,19 +1591,19 @@ public class ConnectivityServiceTest { verifyActiveNetwork(TRANSPORT_WIFI); // Test bringing up validated cellular mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Test cellular disconnect. - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mCellNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Test WiFi disconnect. - cv = registerConnectivityBroadcast(1); + b = registerConnectivityBroadcast(1); mWiFiNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verifyNoNetwork(); } @@ -1584,25 +1611,25 @@ public class ConnectivityServiceTest { public void testUnvalidatedWifiOutscoresUnvalidatedCellular() throws Exception { // Test bringing up unvalidated cellular. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mCellNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Test bringing up unvalidated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Test WiFi disconnect. - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Test cellular disconnect. - cv = registerConnectivityBroadcast(1); + b = registerConnectivityBroadcast(1); mCellNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verifyNoNetwork(); } @@ -1610,24 +1637,24 @@ public class ConnectivityServiceTest { public void testUnlingeringDoesNotValidate() throws Exception { // Test bringing up unvalidated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mWiFiNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); assertFalse(mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); // Test bringing up validated cellular. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); assertFalse(mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); // Test cellular disconnect. - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mCellNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Unlingering a network should not cause it to be marked as validated. assertFalse(mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()).hasCapability( @@ -1638,25 +1665,25 @@ public class ConnectivityServiceTest { public void testCellularOutscoresWeakWifi() throws Exception { // Test bringing up validated cellular. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Test bringing up validated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Test WiFi getting really weak. - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.adjustScore(-11); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Test WiFi restoring signal strength. - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.adjustScore(11); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); } @@ -1674,9 +1701,9 @@ public class ConnectivityServiceTest { mCellNetworkAgent.expectDisconnected(); // Test bringing up validated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - final ConditionVariable cv = registerConnectivityBroadcast(1); + final ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.CONNECTED); mWiFiNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Test bringing up unvalidated cellular. // Expect it to be torn down because it could never be the highest scoring network @@ -1693,33 +1720,33 @@ public class ConnectivityServiceTest { public void testCellularFallback() throws Exception { // Test bringing up validated cellular. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Test bringing up validated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Reevaluate WiFi (it'll instantly fail DNS). - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); assertTrue(mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); mCm.reportBadNetwork(mWiFiNetworkAgent.getNetwork()); // Should quickly fall back to Cellular. - waitFor(cv); + b.expectBroadcast(); assertFalse(mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); verifyActiveNetwork(TRANSPORT_CELLULAR); // Reevaluate cellular (it'll instantly fail DNS). - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); assertTrue(mCm.getNetworkCapabilities(mCellNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); mCm.reportBadNetwork(mCellNetworkAgent.getNetwork()); // Should quickly fall back to WiFi. - waitFor(cv); + b.expectBroadcast(); assertFalse(mCm.getNetworkCapabilities(mCellNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); assertFalse(mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()).hasCapability( @@ -1731,23 +1758,23 @@ public class ConnectivityServiceTest { public void testWiFiFallback() throws Exception { // Test bringing up unvalidated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mWiFiNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Test bringing up validated cellular. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Reevaluate cellular (it'll instantly fail DNS). - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); assertTrue(mCm.getNetworkCapabilities(mCellNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); mCm.reportBadNetwork(mCellNetworkAgent.getNetwork()); // Should quickly fall back to WiFi. - waitFor(cv); + b.expectBroadcast(); assertFalse(mCm.getNetworkCapabilities(mCellNetworkAgent.getNetwork()).hasCapability( NET_CAPABILITY_VALIDATED)); verifyActiveNetwork(TRANSPORT_WIFI); @@ -1817,13 +1844,13 @@ public class ConnectivityServiceTest { mCm.registerNetworkCallback(cellRequest, cellNetworkCallback); // Test unvalidated networks - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); mCellNetworkAgent.connect(false); genericNetworkCallback.expectAvailableCallbacksUnvalidated(mCellNetworkAgent); cellNetworkCallback.expectAvailableCallbacksUnvalidated(mCellNetworkAgent); assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - waitFor(cv); + b.expectBroadcast(); assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback); // This should not trigger spurious onAvailable() callbacks, b/21762680. @@ -1832,28 +1859,28 @@ public class ConnectivityServiceTest { assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback); assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(false); genericNetworkCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); wifiNetworkCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - waitFor(cv); + b.expectBroadcast(); assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback); - cv = registerConnectivityBroadcast(2); + b = registerConnectivityBroadcast(2); mWiFiNetworkAgent.disconnect(); genericNetworkCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); wifiNetworkCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); cellNetworkCallback.assertNoCallback(); - waitFor(cv); + b.expectBroadcast(); assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback); - cv = registerConnectivityBroadcast(1); + b = registerConnectivityBroadcast(1); mCellNetworkAgent.disconnect(); genericNetworkCallback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent); cellNetworkCallback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent); - waitFor(cv); + b.expectBroadcast(); assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback); // Test validated networks @@ -2542,9 +2569,9 @@ public class ConnectivityServiceTest { // Test bringing up validated WiFi. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - final ConditionVariable cv = registerConnectivityBroadcast(1); + final ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.CONNECTED); mWiFiNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); // Register MMS NetworkRequest @@ -2570,9 +2597,9 @@ public class ConnectivityServiceTest { public void testMMSonCell() throws Exception { // Test bringing up cellular without MMS mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = expectConnectivityAction(TYPE_MOBILE, DetailedState.CONNECTED); mCellNetworkAgent.connect(false); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_CELLULAR); // Register MMS NetworkRequest @@ -4037,9 +4064,9 @@ public class ConnectivityServiceTest { } mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.CONNECTED); mWiFiNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); verifyActiveNetwork(TRANSPORT_WIFI); mWiFiNetworkAgent.sendLinkProperties(lp); waitForIdle(); @@ -4570,10 +4597,10 @@ public class ConnectivityServiceTest { assertNotPinnedToWifi(); // Disconnect cell and wifi. - ConditionVariable cv = registerConnectivityBroadcast(3); // cell down, wifi up, wifi down. + ExpectedBroadcast b = registerConnectivityBroadcast(3); // cell down, wifi up, wifi down. mCellNetworkAgent.disconnect(); mWiFiNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); // Pinning takes effect even if the pinned network is the default when the pin is set... TestNetworkPinner.pin(mServiceContext, wifiRequest); @@ -4583,10 +4610,10 @@ public class ConnectivityServiceTest { assertPinnedToWifiWithWifiDefault(); // ... and is maintained even when that network is no longer the default. - cv = registerConnectivityBroadcast(1); + b = registerConnectivityBroadcast(1); mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mCellNetworkAgent.connect(true); - waitFor(cv); + b.expectBroadcast(); assertPinnedToWifiWithCellDefault(); } @@ -4684,7 +4711,7 @@ public class ConnectivityServiceTest { @Test public void testNetworkInfoOfTypeNone() throws Exception { - ConditionVariable broadcastCV = registerConnectivityBroadcast(1); + ExpectedBroadcast b = registerConnectivityBroadcast(1); verifyNoNetwork(); TestNetworkAgentWrapper wifiAware = new TestNetworkAgentWrapper(TRANSPORT_WIFI_AWARE); @@ -4717,9 +4744,7 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(callback); verifyNoNetwork(); - if (broadcastCV.block(10)) { - fail("expected no broadcast, but got CONNECTIVITY_ACTION broadcast"); - } + b.expectNoBroadcast(10); } @Test @@ -6456,11 +6481,11 @@ public class ConnectivityServiceTest { // prefix discovery is never started. LinkProperties lp = new LinkProperties(baseLp); lp.setNat64Prefix(pref64FromRa); - mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, lp); - mCellNetworkAgent.connect(false); - final Network network = mCellNetworkAgent.getNetwork(); + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, lp); + mWiFiNetworkAgent.connect(false); + final Network network = mWiFiNetworkAgent.getNetwork(); int netId = network.getNetId(); - callback.expectAvailableCallbacksUnvalidated(mCellNetworkAgent); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); inOrder.verify(mMockNetd).clatdStart(iface, pref64FromRa.toString()); inOrder.verify(mMockDnsResolver).setPrefix64(netId, pref64FromRa.toString()); inOrder.verify(mMockDnsResolver, never()).startPrefix64Discovery(netId); @@ -6469,8 +6494,8 @@ public class ConnectivityServiceTest { // If the RA prefix is withdrawn, clatd is stopped and prefix discovery is started. lp.setNat64Prefix(null); - mCellNetworkAgent.sendLinkProperties(lp); - expectNat64PrefixChange(callback, mCellNetworkAgent, null); + mWiFiNetworkAgent.sendLinkProperties(lp); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, null); inOrder.verify(mMockNetd).clatdStop(iface); inOrder.verify(mMockDnsResolver).setPrefix64(netId, ""); inOrder.verify(mMockDnsResolver).startPrefix64Discovery(netId); @@ -6478,8 +6503,8 @@ public class ConnectivityServiceTest { // If the RA prefix appears while DNS discovery is in progress, discovery is stopped and // clatd is started with the prefix from the RA. lp.setNat64Prefix(pref64FromRa); - mCellNetworkAgent.sendLinkProperties(lp); - expectNat64PrefixChange(callback, mCellNetworkAgent, pref64FromRa); + mWiFiNetworkAgent.sendLinkProperties(lp); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, pref64FromRa); inOrder.verify(mMockNetd).clatdStart(iface, pref64FromRa.toString()); inOrder.verify(mMockDnsResolver).stopPrefix64Discovery(netId); inOrder.verify(mMockDnsResolver).setPrefix64(netId, pref64FromRa.toString()); @@ -6487,21 +6512,21 @@ public class ConnectivityServiceTest { // Withdraw the RA prefix so we can test the case where an RA prefix appears after DNS // discovery has succeeded. lp.setNat64Prefix(null); - mCellNetworkAgent.sendLinkProperties(lp); - expectNat64PrefixChange(callback, mCellNetworkAgent, null); + mWiFiNetworkAgent.sendLinkProperties(lp); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, null); inOrder.verify(mMockNetd).clatdStop(iface); inOrder.verify(mMockDnsResolver).setPrefix64(netId, ""); inOrder.verify(mMockDnsResolver).startPrefix64Discovery(netId); mService.mNetdEventCallback.onNat64PrefixEvent(netId, true /* added */, pref64FromDnsStr, 96); - expectNat64PrefixChange(callback, mCellNetworkAgent, pref64FromDns); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, pref64FromDns); inOrder.verify(mMockNetd).clatdStart(iface, pref64FromDns.toString()); // If an RA advertises the same prefix that was discovered by DNS, nothing happens: prefix // discovery is not stopped, and there are no callbacks. lp.setNat64Prefix(pref64FromDns); - mCellNetworkAgent.sendLinkProperties(lp); + mWiFiNetworkAgent.sendLinkProperties(lp); callback.assertNoCallback(); inOrder.verify(mMockNetd, never()).clatdStop(iface); inOrder.verify(mMockNetd, never()).clatdStart(eq(iface), anyString()); @@ -6511,7 +6536,7 @@ public class ConnectivityServiceTest { // If the RA is later withdrawn, nothing happens again. lp.setNat64Prefix(null); - mCellNetworkAgent.sendLinkProperties(lp); + mWiFiNetworkAgent.sendLinkProperties(lp); callback.assertNoCallback(); inOrder.verify(mMockNetd, never()).clatdStop(iface); inOrder.verify(mMockNetd, never()).clatdStart(eq(iface), anyString()); @@ -6521,8 +6546,8 @@ public class ConnectivityServiceTest { // If the RA prefix changes, clatd is restarted and prefix discovery is stopped. lp.setNat64Prefix(pref64FromRa); - mCellNetworkAgent.sendLinkProperties(lp); - expectNat64PrefixChange(callback, mCellNetworkAgent, pref64FromRa); + mWiFiNetworkAgent.sendLinkProperties(lp); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, pref64FromRa); inOrder.verify(mMockNetd).clatdStop(iface); inOrder.verify(mMockDnsResolver).stopPrefix64Discovery(netId); @@ -6536,8 +6561,8 @@ public class ConnectivityServiceTest { // If the RA prefix changes, clatd is restarted and prefix discovery is not started. lp.setNat64Prefix(newPref64FromRa); - mCellNetworkAgent.sendLinkProperties(lp); - expectNat64PrefixChange(callback, mCellNetworkAgent, newPref64FromRa); + mWiFiNetworkAgent.sendLinkProperties(lp); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, newPref64FromRa); inOrder.verify(mMockNetd).clatdStop(iface); inOrder.verify(mMockDnsResolver).setPrefix64(netId, ""); inOrder.verify(mMockNetd).clatdStart(iface, newPref64FromRa.toString()); @@ -6547,7 +6572,7 @@ public class ConnectivityServiceTest { // If the RA prefix changes to the same value, nothing happens. lp.setNat64Prefix(newPref64FromRa); - mCellNetworkAgent.sendLinkProperties(lp); + mWiFiNetworkAgent.sendLinkProperties(lp); callback.assertNoCallback(); assertEquals(newPref64FromRa, mCm.getLinkProperties(network).getNat64Prefix()); inOrder.verify(mMockNetd, never()).clatdStop(iface); @@ -6561,19 +6586,19 @@ public class ConnectivityServiceTest { // If the same prefix is learned first by DNS and then by RA, and clat is later stopped, // (e.g., because the network disconnects) setPrefix64(netid, "") is never called. lp.setNat64Prefix(null); - mCellNetworkAgent.sendLinkProperties(lp); - expectNat64PrefixChange(callback, mCellNetworkAgent, null); + mWiFiNetworkAgent.sendLinkProperties(lp); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, null); inOrder.verify(mMockNetd).clatdStop(iface); inOrder.verify(mMockDnsResolver).setPrefix64(netId, ""); inOrder.verify(mMockDnsResolver).startPrefix64Discovery(netId); mService.mNetdEventCallback.onNat64PrefixEvent(netId, true /* added */, pref64FromDnsStr, 96); - expectNat64PrefixChange(callback, mCellNetworkAgent, pref64FromDns); + expectNat64PrefixChange(callback, mWiFiNetworkAgent, pref64FromDns); inOrder.verify(mMockNetd).clatdStart(iface, pref64FromDns.toString()); inOrder.verify(mMockDnsResolver, never()).setPrefix64(eq(netId), any()); lp.setNat64Prefix(pref64FromDns); - mCellNetworkAgent.sendLinkProperties(lp); + mWiFiNetworkAgent.sendLinkProperties(lp); callback.assertNoCallback(); inOrder.verify(mMockNetd, never()).clatdStop(iface); inOrder.verify(mMockNetd, never()).clatdStart(eq(iface), anyString()); @@ -6584,10 +6609,10 @@ public class ConnectivityServiceTest { // When tearing down a network, clat state is only updated after CALLBACK_LOST is fired, but // before CONNECTIVITY_ACTION is sent. Wait for CONNECTIVITY_ACTION before verifying that // clat has been stopped, or the test will be flaky. - ConditionVariable cv = registerConnectivityBroadcast(1); - mCellNetworkAgent.disconnect(); - callback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent); - waitFor(cv); + ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.DISCONNECTED); + mWiFiNetworkAgent.disconnect(); + callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); + b.expectBroadcast(); inOrder.verify(mMockNetd).clatdStop(iface); inOrder.verify(mMockDnsResolver).stopPrefix64Discovery(netId); @@ -6662,10 +6687,10 @@ public class ConnectivityServiceTest { .destroyNetworkCache(eq(mCellNetworkAgent.getNetwork().netId)); // Disconnect wifi - ConditionVariable cv = registerConnectivityBroadcast(1); + ExpectedBroadcast b = expectConnectivityAction(TYPE_WIFI, DetailedState.DISCONNECTED); reset(mNetworkManagementService); mWiFiNetworkAgent.disconnect(); - waitFor(cv); + b.expectBroadcast(); verify(mNetworkManagementService, times(1)).removeIdleTimer(eq(WIFI_IFNAME)); // Clean up From ab175f7f6decfbda65aa4402eb5d43ba4bfbea1e Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 15 Dec 2020 00:45:30 +0900 Subject: [PATCH 19/24] Add a test for getDefaultNetworkCapabilitiesForUser. Bug: 172870110 Test: test-only change Test: new test passes 100 times in a row Change-Id: I210284578e38cd25b8b95235d3390d5bd66a5a70 Merged-In: I210284578e38cd25b8b95235d3390d5bd66a5a70 (cherry picked from commit a9aca9744745b7fc8b3d2a1235096982ad8ec8b6) --- .../server/ConnectivityServiceTest.java | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 6cbcb6784ddd6..fb370a0f7124b 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5667,10 +5667,21 @@ public class ConnectivityServiceTest { assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); } + private void assertDefaultNetworkCapabilities(int userId, NetworkAgentWrapper... networks) { + final NetworkCapabilities[] defaultCaps = mService.getDefaultNetworkCapabilitiesForUser( + userId, "com.android.calling.package"); + final String defaultCapsString = Arrays.toString(defaultCaps); + assertEquals(defaultCapsString, defaultCaps.length, networks.length); + final Set defaultCapsSet = new ArraySet<>(defaultCaps); + for (NetworkAgentWrapper network : networks) { + final NetworkCapabilities nc = mCm.getNetworkCapabilities(network.getNetwork()); + final String msg = "Did not find " + nc + " in " + Arrays.toString(defaultCaps); + assertTrue(msg, defaultCapsSet.contains(nc)); + } + } + @Test public void testVpnSetUnderlyingNetworks() throws Exception { - final int uid = Process.myUid(); - final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback(); final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder() .removeCapability(NET_CAPABILITY_NOT_VPN) @@ -5693,6 +5704,9 @@ public class ConnectivityServiceTest { // A VPN without underlying networks is not suspended. assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + final int userId = UserHandle.getUserId(Process.myUid()); + assertDefaultNetworkCapabilities(userId /* no networks */); + // Connect cell and use it as an underlying network. mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED); @@ -5706,6 +5720,7 @@ public class ConnectivityServiceTest { && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertDefaultNetworkCapabilities(userId, mCellNetworkAgent); mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); @@ -5720,6 +5735,7 @@ public class ConnectivityServiceTest { && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Don't disconnect, but note the VPN is not using wifi any more. mService.setUnderlyingNetworksForVpn( @@ -5730,6 +5746,9 @@ public class ConnectivityServiceTest { && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + // The return value of getDefaultNetworkCapabilitiesForUser always includes the default + // network (wifi) as well as the underlying networks (cell). + assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Remove NOT_SUSPENDED from the only network and observe VPN is now suspended. mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_SUSPENDED); @@ -5758,6 +5777,7 @@ public class ConnectivityServiceTest { && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertDefaultNetworkCapabilities(userId, mWiFiNetworkAgent); // Use both again. mService.setUnderlyingNetworksForVpn( @@ -5768,6 +5788,7 @@ public class ConnectivityServiceTest { && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Cell is suspended again. As WiFi is not, this should not cause a callback. mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_SUSPENDED); @@ -5785,6 +5806,7 @@ public class ConnectivityServiceTest { // a bug in ConnectivityService, but as the SUSPENDED and RESUMED callbacks have never // been public and are deprecated and slated for removal, there is no sense in spending // resources fixing this bug now. + assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Use both again. mService.setUnderlyingNetworksForVpn( @@ -5797,6 +5819,7 @@ public class ConnectivityServiceTest { && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); // As above, the RESUMED callback not being sent here is a bug, but not a bug that's // worth anybody's time to fix. + assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Disconnect cell. Receive update without even removing the dead network from the // underlying networks – it's dead anyway. Not metered any more. @@ -5805,6 +5828,7 @@ public class ConnectivityServiceTest { (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && caps.hasCapability(NET_CAPABILITY_NOT_METERED)); + assertDefaultNetworkCapabilities(userId, mWiFiNetworkAgent); // Disconnect wifi too. No underlying networks means this is now metered. mWiFiNetworkAgent.disconnect(); @@ -5812,6 +5836,11 @@ public class ConnectivityServiceTest { (caps) -> caps.hasTransport(TRANSPORT_VPN) && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)); + // When a network disconnects, the callbacks are fired before all state is updated, so for a + // short time, synchronous calls will behave as if the network is still connected. Wait for + // things to settle. + waitForIdle(); + assertDefaultNetworkCapabilities(userId /* no networks */); mMockVpn.disconnect(); } From 0f3f49a660c78b0336c85fcf7119ad498312f2cb Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 7 Jan 2021 23:33:18 +0900 Subject: [PATCH 20/24] Test for bugs with suspended VPN underlying networks. Bug: 172870110 Test: atest --rerun-until-failure 100 ConnectivityServiceTest#testVpnSwitchFromSuspendedToNonSuspended Change-Id: Ia52f9cafef3f49ae70ad135d017e207eb57fddfe Merged-In: Ia52f9cafef3f49ae70ad135d017e207eb57fddfe (cherry picked from commit 27ecde937319012a3fc07783e1133e14111da788) --- .../server/ConnectivityServiceTest.java | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index fb370a0f7124b..ffd6b28e8b0d5 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5433,6 +5433,121 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(callback); } + private void assertGetNetworkInfoOfGetActiveNetworkIsConnected(boolean expectedConnectivity) { + // What Chromium used to do before https://chromium-review.googlesource.com/2605304 + assertEquals("Unexpected result for getActiveNetworkInfo(getActiveNetwork())", + expectedConnectivity, mCm.getNetworkInfo(mCm.getActiveNetwork()).isConnected()); + } + + @Test + public void testVpnUnderlyingNetworkSuspended() throws Exception { + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(callback); + + // Connect a VPN. + mMockVpn.establishForMyUid(false, true, false); + callback.expectAvailableCallbacksUnvalidated(mMockVpn); + + // Connect cellular data. + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); + mCellNetworkAgent.connect(false /* validated */); + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.assertNoCallback(); + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertGetNetworkInfoOfGetActiveNetworkIsConnected(true); + + // Suspend the cellular network and expect the VPN to be suspended. + mCellNetworkAgent.suspend(); + callback.expectCapabilitiesThat(mMockVpn, + nc -> !nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn); + callback.assertNoCallback(); + + assertFalse(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); + // VPN's main underlying network is suspended, so no connectivity. + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + // Switch to another network. The VPN should no longer be suspended. + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(false /* validated */); + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_WIFI)); + + // BUG: the VPN is no longer suspended, so a RESUMED callback should have been sent. + // callback.expectCallback(CallbackEntry.RESUMED, mMockVpn); + callback.assertNoCallback(); + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. + assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); + // BUG: the device has connectivity, so this should return true. + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + // Unsuspend cellular and then switch back to it. + // The same bug happens in the opposite direction: the VPN's capabilities correctly have + // NOT_SUSPENDED, but the VPN's NetworkInfo is in state SUSPENDED. + mCellNetworkAgent.resume(); + mWiFiNetworkAgent.disconnect(); + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + // Spurious double callback? + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.assertNoCallback(); + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. + assertNull(mCm.getActiveNetworkInfo()); // ??? + // BUG: the device has connectivity, so this should return true. + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + // Suspending and resuming reveals other bugs. + mCellNetworkAgent.suspend(); + callback.assertNoCallback(); // BUG: should get callback that VPN is suspended. + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); // BUG: VPN should be SUSPENDED. + assertNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); + assertNull(mCm.getActiveNetworkInfo()); // ??? + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + mCellNetworkAgent.resume(); + callback.assertNoCallback(); // BUG: should get callback that VPN is no longer suspended. + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); + assertNull(mCm.getActiveNetworkInfo()); // ??? + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + } + @Test public void testVpnNetworkActive() throws Exception { final int uid = Process.myUid(); From 7b6707b4844afdc0cda74e1ed623d6a93693000c Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 28 Jan 2021 18:31:31 +0900 Subject: [PATCH 21/24] Backport some helpers in ConnectivityServiceTest. These were added in aosp/1527378, which is impractical to backport. Bug: 172870110 Test: test-only change Change-Id: Id3d12b23034b284c8f7dffb5167244e1e43987e2 Merged-In: I827543751dbf5e626a24ec02cd6f50b423f5f761 (cherry picked from commit 4c36cb764a6ffbc81acdbf2297cde010a7396f8b) --- .../android/server/ConnectivityServiceTest.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index ffd6b28e8b0d5..d8e1f0a434f1d 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -36,6 +36,7 @@ import static android.net.ConnectivityManager.TYPE_MOBILE; import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA; import static android.net.ConnectivityManager.TYPE_MOBILE_MMS; import static android.net.ConnectivityManager.TYPE_MOBILE_SUPL; +import static android.net.ConnectivityManager.TYPE_VPN; import static android.net.ConnectivityManager.TYPE_WIFI; import static android.net.INetworkMonitor.NETWORK_VALIDATION_PROBE_DNS; import static android.net.INetworkMonitor.NETWORK_VALIDATION_PROBE_FALLBACK; @@ -6289,6 +6290,19 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(defaultCallback); } + private void checkNetworkInfo(NetworkInfo ni, int type, DetailedState state) { + assertNotNull(ni); + assertEquals(type, ni.getType()); + assertEquals(ConnectivityManager.getNetworkTypeName(type), state, ni.getDetailedState()); + } + + private void assertActiveNetworkInfo(int type, DetailedState state) { + checkNetworkInfo(mCm.getActiveNetworkInfo(), type, state); + } + private void assertNetworkInfo(int type, DetailedState state) { + checkNetworkInfo(mCm.getNetworkInfo(type), type, state); + } + @Test public final void testLoseTrusted() throws Exception { final NetworkRequest trustedRequest = new NetworkRequest.Builder() From c5c0b99e7f003c663ce1e9635106d8dc4d6db4ae Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 12 Jan 2021 00:34:44 +0900 Subject: [PATCH 22/24] Backport test coverage from aosp/1547496. This test coverage is necessary to fix an upcoming bug in R. Backport it from the change that added it. The non-test portion of that change is not necessary in R because it fixes a bug that was introduced in S. Bug: 172870110 Test: accompanying unit test shows lots of bugs removed Change-Id: If7eb8857474d8b4f774f5fa5db2a3112e85c9cae Merged-In: Ibf376a6fa4b34d1c96f8506fa8abbb7595a8c272 (cherry picked from commit 55229beb747b9e7b8a0f392d855a1b3342df8646) --- .../server/ConnectivityServiceTest.java | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index d8e1f0a434f1d..3d4ff41d65fbc 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5521,32 +5521,40 @@ public class ConnectivityServiceTest { assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. - assertNull(mCm.getActiveNetworkInfo()); // ??? + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); // BUG: the device has connectivity, so this should return true. assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); - // Suspending and resuming reveals other bugs. + // Re-suspending the current network fixes the problem. mCellNetworkAgent.suspend(); - callback.assertNoCallback(); // BUG: should get callback that VPN is suspended. + callback.expectCapabilitiesThat(mMockVpn, + nc -> !nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn); + callback.assertNoCallback(); - assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) - .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); // BUG: VPN should be SUSPENDED. + assertFalse(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); assertNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); - assertNull(mCm.getActiveNetworkInfo()); // ??? + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); mCellNetworkAgent.resume(); - callback.assertNoCallback(); // BUG: should get callback that VPN is no longer suspended. + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.expectCallback(CallbackEntry.RESUMED, mMockVpn); + callback.assertNoCallback(); assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); - assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); - assertNull(mCm.getActiveNetworkInfo()); // ??? - assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertGetNetworkInfoOfGetActiveNetworkIsConnected(true); } @Test From 77a098a332ebb5c62d28b88a5ee0b7f87372c021 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 11 Jan 2021 22:27:57 +0900 Subject: [PATCH 23/24] Fix legacy APIs when VPN switches to suspended underlying network. Currently, when the VPN underlying network changes from a network that is not suspended to one that is suspended (or vice versa), some of the legacy APIs return incorrect results. This is because the VPN's NetworkInfo can get into SUSPENDED state even though the capabilities have the NOT_SUSPENDED capability. This happens because the code in updateCapabilities that checks for changes in NOT_SUSPENDED and NOT_ROAMING (which are the capabilities that can affect the NetworkInfo state) is only run when the capabilities change in a certain way. Fix this by always checking for changes in these capabilities, regardless of what else has changed. This results in sending a lot more SUSPENDED and RESUMED callbacks than the code sent previously. This should hopefully not impact apps because those callback methods have never been public API, though because they're just callbacks, it's possible that apps found out via code inspection that the callbacks existed and implemented them. Bug: 172870110 Test: changes to existing tests in ConnectivityServiceTest Change-Id: I6ec246a6a4e61f634956a165797fbb80296efd6a Merged-In: I6ec246a6a4e61f634956a165797fbb80296efd6a (cherry picked from commit 5b90ebaf4d9edefcd9648b46cd0226f882169476) --- .../android/server/ConnectivityService.java | 33 +++++++++++-------- .../server/ConnectivityServiceTest.java | 29 ++++++---------- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index cf14c6377d210..a1cbd00e360f1 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6318,6 +6318,25 @@ public class ConnectivityService extends IConnectivityManager.Stub return newNc; } + private void updateNetworkInfoForRoamingAndSuspended(NetworkAgentInfo nai, + NetworkCapabilities prevNc, NetworkCapabilities newNc) { + final boolean prevSuspended = !prevNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED); + final boolean suspended = !newNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED); + final boolean prevRoaming = !prevNc.hasCapability(NET_CAPABILITY_NOT_ROAMING); + final boolean roaming = !newNc.hasCapability(NET_CAPABILITY_NOT_ROAMING); + if (prevSuspended != suspended) { + // TODO (b/73132094) : remove this call once the few users of onSuspended and + // onResumed have been removed. + notifyNetworkCallbacks(nai, suspended ? ConnectivityManager.CALLBACK_SUSPENDED + : ConnectivityManager.CALLBACK_RESUMED); + } + if (prevSuspended != suspended || prevRoaming != roaming) { + // updateNetworkInfo will mix in the suspended info from the capabilities and + // take appropriate action for the network having possibly changed state. + updateNetworkInfo(nai, nai.networkInfo); + } + } + /** * Update the NetworkCapabilities for {@code nai} to {@code nc}. Specifically: * @@ -6349,25 +6368,13 @@ public class ConnectivityService extends IConnectivityManager.Stub // on this network. We might have been called by rematchNetworkAndRequests when a // network changed foreground state. processListenRequests(nai); - final boolean prevSuspended = !prevNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED); - final boolean suspended = !newNc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED); - final boolean prevRoaming = !prevNc.hasCapability(NET_CAPABILITY_NOT_ROAMING); - final boolean roaming = !newNc.hasCapability(NET_CAPABILITY_NOT_ROAMING); - if (prevSuspended != suspended || prevRoaming != roaming) { - // TODO (b/73132094) : remove this call once the few users of onSuspended and - // onResumed have been removed. - notifyNetworkCallbacks(nai, suspended ? ConnectivityManager.CALLBACK_SUSPENDED - : ConnectivityManager.CALLBACK_RESUMED); - // updateNetworkInfo will mix in the suspended info from the capabilities and - // take appropriate action for the network having possibly changed state. - updateNetworkInfo(nai, nai.networkInfo); - } } else { // If the requestable capabilities have changed or the score changed, we can't have been // called by rematchNetworkAndRequests, so it's safe to start a rematch. rematchAllNetworksAndRequests(); notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); } + updateNetworkInfoForRoamingAndSuspended(nai, prevNc, newNc); // TODO : static analysis indicates that prevNc can't be null here (getAndSetNetworkCaps // never returns null), so mark the relevant members and functions in nai as @NonNull and diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 3d4ff41d65fbc..a5554c740e7f7 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5488,23 +5488,18 @@ public class ConnectivityServiceTest { callback.expectCapabilitiesThat(mMockVpn, nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) && nc.hasTransport(TRANSPORT_WIFI)); - - // BUG: the VPN is no longer suspended, so a RESUMED callback should have been sent. - // callback.expectCallback(CallbackEntry.RESUMED, mMockVpn); + callback.expectCallback(CallbackEntry.RESUMED, mMockVpn); callback.assertNoCallback(); assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); assertNetworkInfo(TYPE_MOBILE, DetailedState.DISCONNECTED); assertNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); - assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. + assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); - // BUG: the device has connectivity, so this should return true. - assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + assertGetNetworkInfoOfGetActiveNetworkIsConnected(true); - // Unsuspend cellular and then switch back to it. - // The same bug happens in the opposite direction: the VPN's capabilities correctly have - // NOT_SUSPENDED, but the VPN's NetworkInfo is in state SUSPENDED. + // Unsuspend cellular and then switch back to it. The VPN remains not suspended. mCellNetworkAgent.resume(); mWiFiNetworkAgent.disconnect(); callback.expectCapabilitiesThat(mMockVpn, @@ -5520,12 +5515,11 @@ public class ConnectivityServiceTest { .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); - assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. + assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED); assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); - // BUG: the device has connectivity, so this should return true. - assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + assertGetNetworkInfoOfGetActiveNetworkIsConnected(true); - // Re-suspending the current network fixes the problem. + // Suspend cellular and expect no connectivity. mCellNetworkAgent.suspend(); callback.expectCapabilitiesThat(mMockVpn, nc -> !nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) @@ -5541,6 +5535,7 @@ public class ConnectivityServiceTest { assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + // Resume cellular and expect that connectivity comes back. mCellNetworkAgent.resume(); callback.expectCapabilitiesThat(mMockVpn, nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) @@ -5926,10 +5921,7 @@ public class ConnectivityServiceTest { && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && !caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); - // While the SUSPENDED callback should in theory be sent here, it is not. This is - // a bug in ConnectivityService, but as the SUSPENDED and RESUMED callbacks have never - // been public and are deprecated and slated for removal, there is no sense in spending - // resources fixing this bug now. + vpnNetworkCallback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn); assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Use both again. @@ -5941,8 +5933,7 @@ public class ConnectivityServiceTest { && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) && !caps.hasCapability(NET_CAPABILITY_NOT_METERED) && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); - // As above, the RESUMED callback not being sent here is a bug, but not a bug that's - // worth anybody's time to fix. + vpnNetworkCallback.expectCallback(CallbackEntry.RESUMED, mMockVpn); assertDefaultNetworkCapabilities(userId, mCellNetworkAgent, mWiFiNetworkAgent); // Disconnect cell. Receive update without even removing the dead network from the From 70b6d10c3de3e211f3ec6d1a5b559c6a4c660218 Mon Sep 17 00:00:00 2001 From: Winson Chiu Date: Mon, 1 Mar 2021 18:12:13 +0000 Subject: [PATCH 24/24] Revert "Only allow BROWSABLE && DEFAULT Intents to be always opened" Reason for revert: Punted to future release due to invalid fix Bug: 175319005 Merged-In: I00b78d596ee05c5a4a228771bbf8082af2b0ab8a Change-Id: I78284e0a0dd5c41345753cdd2ed9a518db1df930 (cherry picked from commit ffe45e449799c62ee43a5cf58f87f115478bc5cf) --- .../java/com/android/server/pm/PackageManagerService.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 40723e073ba10..9ac2da188babc 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -7706,13 +7706,6 @@ public class PackageManagerService extends IPackageManager.Stub Slog.i(TAG, " + always: " + info.activityInfo.packageName + " : linkgen=" + linkGeneration); } - - if (!intent.hasCategory(CATEGORY_BROWSABLE) - || !intent.hasCategory(CATEGORY_DEFAULT)) { - undefinedList.add(info); - continue; - } - // Use link-enabled generation as preferredOrder, i.e. // prefer newly-enabled over earlier-enabled. info.preferredOrder = linkGeneration;