From 455277943e458b1bcdfe0d91f3b2748d0a481aed Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Tue, 25 Jun 2019 09:40:54 -0700 Subject: [PATCH] Only show "Connected" note after opening portal The "Connected" notification would be shown every time a network validates after being identified as a captive portal. This causes issues on networks that have auto-login mechanisms, as a high priority notification would be shown even though the user was not interacting with the phone. The "Connected" notification is intended to confirm to the user that they successfuly logged in (manually), so only show it after the user opens the portal on the network. Bug: 134124044 Test: Flashed, connected to portal: notification shown Opened portal from command line + revalidate: no notification Tests passing with change, failing without Merged-In: I99be7d312d020d242081971c7e522023bbbab072 Merged-In: I7dc1b3a313b255fe89313efb9117bb160efdb533 (cherry picked from commit 0b5a4d862190320d285413b1feb921144fee8420) Change-Id: I67c124cc34f09c2f186706b5cec839f60d00a90a --- .../com/android/server/ConnectivityService.java | 17 +++++---- .../server/connectivity/NetworkAgentInfo.java | 8 ++--- .../android/server/ConnectivityServiceTest.java | 42 +++++++++++++++++++--- 3 files changed, 52 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index e81d1721b271..fc67c38560fa 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2616,9 +2616,11 @@ public class ConnectivityService extends IConnectivityManager.Stub final boolean valid = ((msg.arg1 & NETWORK_VALIDATION_RESULT_VALID) != 0); final boolean wasValidated = nai.lastValidated; final boolean wasDefault = isDefaultNetwork(nai); - if (nai.everCaptivePortalDetected && !nai.captivePortalLoginNotified - && valid) { - nai.captivePortalLoginNotified = true; + // Only show a connected notification if the network is pending validation + // after the captive portal app was open, and it has now validated. + if (nai.captivePortalValidationPending && valid) { + // User is now logged in, network validated. + nai.captivePortalValidationPending = false; showNetworkNotification(nai, NotificationType.LOGGED_IN); } @@ -2689,9 +2691,6 @@ public class ConnectivityService extends IConnectivityManager.Stub final int oldScore = nai.getCurrentScore(); nai.lastCaptivePortalDetected = visible; nai.everCaptivePortalDetected |= visible; - if (visible) { - nai.captivePortalLoginNotified = false; - } if (nai.lastCaptivePortalDetected && Settings.Global.CAPTIVE_PORTAL_MODE_AVOID == getCaptivePortalMode()) { if (DBG) log("Avoiding captive portal network: " + nai.name()); @@ -3500,6 +3499,12 @@ public class ConnectivityService extends IConnectivityManager.Stub new CaptivePortal(new CaptivePortalImpl(network).asBinder())); appIntent.setFlags(Intent.FLAG_ACTIVITY_BROUGHT_TO_FRONT | Intent.FLAG_ACTIVITY_NEW_TASK); + // This runs on a random binder thread, but getNetworkAgentInfoForNetwork is thread-safe, + // and captivePortalValidationPending is volatile. + final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network); + if (nai != null) { + nai.captivePortalValidationPending = true; + } Binder.withCleanCallingIdentity(() -> mContext.startActivityAsUser(appIntent, UserHandle.CURRENT)); } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 864a793b8f40..5b043799f848 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -155,9 +155,9 @@ public class NetworkAgentInfo implements Comparable { // Whether a captive portal was found during the last network validation attempt. public boolean lastCaptivePortalDetected; - // Indicates the user was notified of a successful captive portal login since a portal was - // last detected. - public boolean captivePortalLoginNotified; + // Indicates the captive portal app was opened to show a login UI to the user, but the network + // has not validated yet. + public volatile boolean captivePortalValidationPending; // Set to true when partial connectivity was detected. public boolean partialConnectivity; @@ -629,7 +629,7 @@ public class NetworkAgentInfo implements Comparable { + "acceptUnvalidated{" + networkMisc.acceptUnvalidated + "} " + "everCaptivePortalDetected{" + everCaptivePortalDetected + "} " + "lastCaptivePortalDetected{" + lastCaptivePortalDetected + "} " - + "captivePortalLoginNotified{" + captivePortalLoginNotified + "} " + + "captivePortalValidationPending{" + captivePortalValidationPending + "} " + "partialConnectivity{" + partialConnectivity + "} " + "acceptPartialConnectivity{" + networkMisc.acceptPartialConnectivity + "} " + "clat{" + clatd + "} " diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 8c024a5a53c7..73ee7f544b5f 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -18,6 +18,7 @@ package com.android.server; import static android.content.pm.PackageManager.GET_PERMISSIONS; import static android.content.pm.PackageManager.MATCH_ANY_USER; +import static android.net.ConnectivityManager.ACTION_CAPTIVE_PORTAL_SIGN_IN; import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.NETID_UNSET; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OFF; @@ -80,13 +81,13 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Matchers.anyInt; import static org.mockito.Mockito.any; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.eq; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -141,6 +142,7 @@ import android.net.NetworkInfo.DetailedState; import android.net.NetworkMisc; import android.net.NetworkRequest; import android.net.NetworkSpecifier; +import android.net.NetworkStack; import android.net.NetworkStackClient; import android.net.NetworkState; import android.net.NetworkUtils; @@ -154,6 +156,7 @@ import android.net.shared.NetworkMonitorUtils; import android.net.shared.PrivateDnsConfig; import android.net.util.MultinetworkPolicyTracker; import android.os.Binder; +import android.os.Bundle; import android.os.ConditionVariable; import android.os.Handler; import android.os.HandlerThread; @@ -191,6 +194,7 @@ import com.android.server.connectivity.DefaultNetworkMetrics; import com.android.server.connectivity.IpConnectivityMetrics; import com.android.server.connectivity.MockableSystemProperties; import com.android.server.connectivity.Nat464Xlat; +import com.android.server.connectivity.NetworkNotificationManager.NotificationType; import com.android.server.connectivity.ProxyTracker; import com.android.server.connectivity.Tethering; import com.android.server.connectivity.Vpn; @@ -282,6 +286,7 @@ public class ConnectivityServiceTest { @Mock NetworkStackClient mNetworkStack; @Mock PackageManager mPackageManager; @Mock UserManager mUserManager; + @Mock NotificationManager mNotificationManager; private ArgumentCaptor mResolverParamsParcelCaptor = ArgumentCaptor.forClass(ResolverParamsParcel.class); @@ -351,7 +356,7 @@ public class ConnectivityServiceTest { @Override public Object getSystemService(String name) { if (Context.CONNECTIVITY_SERVICE.equals(name)) return mCm; - if (Context.NOTIFICATION_SERVICE.equals(name)) return mock(NotificationManager.class); + if (Context.NOTIFICATION_SERVICE.equals(name)) return mNotificationManager; if (Context.NETWORK_STACK_SERVICE.equals(name)) return mNetworkStack; if (Context.USER_SERVICE.equals(name)) return mUserManager; return super.getSystemService(name); @@ -371,7 +376,17 @@ public class ConnectivityServiceTest { public PackageManager getPackageManager() { return mPackageManager; } - } + + @Override + public void enforceCallingOrSelfPermission(String permission, String message) { + // The mainline permission can only be held if signed with the network stack certificate + // Skip testing for this permission. + if (NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK.equals(permission)) return; + // All other permissions should be held by the test or unnecessary: check as normal to + // make sure the code does not rely on unexpected permissions. + super.enforceCallingOrSelfPermission(permission, message); + } + } public void waitForIdle(int timeoutMsAsInt) { long timeoutMs = timeoutMsAsInt; @@ -2856,6 +2871,9 @@ public class ConnectivityServiceTest { // Expect NET_CAPABILITY_VALIDATED onAvailable callback. validatedCallback.expectAvailableDoubleValidatedCallbacks(mWiFiNetworkAgent); + // Expect no notification to be shown when captive portal disappears by itself + verify(mNotificationManager, never()).notifyAsUser( + anyString(), eq(NotificationType.LOGGED_IN.eventId), any(), any()); // Break network connectivity. // Expect NET_CAPABILITY_VALIDATED onLost callback. @@ -2885,6 +2903,8 @@ public class ConnectivityServiceTest { // Check that calling startCaptivePortalApp does nothing. final int fastTimeoutMs = 100; mCm.startCaptivePortalApp(wifiNetwork); + waitForIdle(); + verify(mWiFiNetworkAgent.mNetworkMonitor, never()).launchCaptivePortalApp(); mServiceContext.expectNoStartActivityIntent(fastTimeoutMs); // Turn into a captive portal. @@ -2895,14 +2915,26 @@ public class ConnectivityServiceTest { // Check that startCaptivePortalApp sends the expected command to NetworkMonitor. mCm.startCaptivePortalApp(wifiNetwork); - verify(mWiFiNetworkAgent.mNetworkMonitor, timeout(TIMEOUT_MS).times(1)) - .launchCaptivePortalApp(); + waitForIdle(); + verify(mWiFiNetworkAgent.mNetworkMonitor).launchCaptivePortalApp(); + + // NetworkMonitor uses startCaptivePortal(Network, Bundle) (startCaptivePortalAppInternal) + final Bundle testBundle = new Bundle(); + final String testKey = "testkey"; + final String testValue = "testvalue"; + testBundle.putString(testKey, testValue); + mCm.startCaptivePortalApp(wifiNetwork, testBundle); + final Intent signInIntent = mServiceContext.expectStartActivityIntent(TIMEOUT_MS); + assertEquals(ACTION_CAPTIVE_PORTAL_SIGN_IN, signInIntent.getAction()); + assertEquals(testValue, signInIntent.getStringExtra(testKey)); // Report that the captive portal is dismissed, and check that callbacks are fired mWiFiNetworkAgent.setNetworkValid(); mWiFiNetworkAgent.mNetworkMonitor.forceReevaluation(Process.myUid()); validatedCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent); captivePortalCallback.expectCallback(CallbackState.LOST, mWiFiNetworkAgent); + verify(mNotificationManager, times(1)).notifyAsUser(anyString(), + eq(NotificationType.LOGGED_IN.eventId), any(), eq(UserHandle.ALL)); mCm.unregisterNetworkCallback(validatedCallback); mCm.unregisterNetworkCallback(captivePortalCallback); -- 2.11.0