From f666d0a21a5e57f5a21a6e6346f271725a7dd68f Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 18 May 2018 21:47:45 +0900 Subject: [PATCH] Add tests for setUnderlyingNetworks. Fixes come later. This is complex enough as it is. Clean cherry-pick of ag/4083953 Bug: 79748782 Test: new test passes, old tests still pass Change-Id: If7276fe1f751be7b9c18f689e97699e566e5bde0 Merged-In: I12c948ebeb2b74290908f8320ff77220dc4a9fb9 --- .../java/com/android/server/connectivity/Vpn.java | 12 +- .../android/server/ConnectivityServiceTest.java | 235 +++++++++++++++++++-- 2 files changed, 227 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 2fda08e2a575..dd82950fe020 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -172,10 +172,13 @@ public class Vpn { private PendingIntent mStatusIntent; private volatile boolean mEnableTeardown = true; private final INetworkManagementService mNetd; - private VpnConfig mConfig; - private NetworkAgent mNetworkAgent; + @VisibleForTesting + protected VpnConfig mConfig; + @VisibleForTesting + protected NetworkAgent mNetworkAgent; private final Looper mLooper; - private final NetworkCapabilities mNetworkCapabilities; + @VisibleForTesting + protected final NetworkCapabilities mNetworkCapabilities; private final SystemServices mSystemServices; /** @@ -1071,7 +1074,8 @@ public class Vpn { // Returns true if the VPN has been established and the calling UID is its owner. Used to check // that a call to mutate VPN state is admissible. - private boolean isCallerEstablishedOwnerLocked() { + @VisibleForTesting + protected boolean isCallerEstablishedOwnerLocked() { return isRunningLocked() && Binder.getCallingUid() == mOwnerUID; } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 52104c265812..572cb521edce 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -112,6 +112,7 @@ import android.net.NetworkUtils; import android.net.RouteInfo; import android.net.StringNetworkSpecifier; import android.net.UidRange; +import android.net.VpnService; import android.net.captiveportal.CaptivePortalProbeResult; import android.net.metrics.IpConnectivityLog; import android.net.util.MultinetworkPolicyTracker; @@ -134,6 +135,7 @@ import android.test.mock.MockContentResolver; import android.util.ArraySet; import android.util.Log; +import com.android.internal.net.VpnConfig; import com.android.internal.util.ArrayUtils; import com.android.internal.util.WakeupMessage; import com.android.internal.util.test.BroadcastInterceptingContext; @@ -197,13 +199,13 @@ public class ConnectivityServiceTest { private MockNetworkAgent mWiFiNetworkAgent; private MockNetworkAgent mCellNetworkAgent; private MockNetworkAgent mEthernetNetworkAgent; + private MockVpn mMockVpn; private Context mContext; @Mock IpConnectivityMetrics.Logger mMetricsService; @Mock DefaultNetworkMetrics mDefaultNetworkMetrics; @Mock INetworkManagementService mNetworkManagementService; @Mock INetworkStatsService mStatsService; - @Mock Vpn mMockVpn; private ArgumentCaptor mStringArrayCaptor = ArgumentCaptor.forClass(String[].class); @@ -479,6 +481,14 @@ public class ConnectivityServiceTest { mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); } + public void setNetworkCapabilities(NetworkCapabilities nc, + boolean sendToConnectivityService) { + mNetworkCapabilities.set(nc); + if (sendToConnectivityService) { + mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); + } + } + public void connectWithoutInternet() { mNetworkInfo.setDetailedState(DetailedState.CONNECTED, null, null); mNetworkAgent.sendNetworkInfo(mNetworkInfo); @@ -594,6 +604,10 @@ public class ConnectivityServiceTest { return mRedirectUrl; } + public NetworkAgent getNetworkAgent() { + return mNetworkAgent; + } + public NetworkCapabilities getNetworkCapabilities() { return mNetworkCapabilities; } @@ -726,6 +740,87 @@ public class ConnectivityServiceTest { } } + 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; + // Careful ! This is different from mNetworkAgent, because MockNetworkAgent does + // not inherit from NetworkAgent. + private MockNetworkAgent mMockNetworkAgent; + + public MockVpn(int userId) { + super(startHandlerThreadAndReturnLooper(), mServiceContext, mNetworkManagementService, + userId); + } + + public void setNetworkAgent(MockNetworkAgent agent) { + waitForIdle(agent, TIMEOUT_MS); + mMockNetworkAgent = agent; + mNetworkAgent = agent.getNetworkAgent(); + mNetworkCapabilities.set(agent.getNetworkCapabilities()); + } + + public void setUids(Set uids) { + mNetworkCapabilities.setUids(uids); + updateCapabilities(); + } + + @Override + public int getNetId() { + return mMockNetworkAgent.getNetwork().netId; + } + + @Override + public boolean appliesToUid(int uid) { + return mConnected; // Trickery to simplify testing. + } + + @Override + protected boolean isCallerEstablishedOwnerLocked() { + return mConnected; // Similar trickery + } + + public void connect() { + mNetworkCapabilities.set(mMockNetworkAgent.getNetworkCapabilities()); + mConnected = true; + mConfig = new VpnConfig(); + } + + @Override + public void updateCapabilities() { + if (!mConnected) return; + super.updateCapabilities(); + // Because super.updateCapabilities will update the capabilities of the agent but not + // the mock agent, the mock agent needs to know about them. + copyCapabilitiesToNetworkAgent(); + } + + private void copyCapabilitiesToNetworkAgent() { + if (null != mMockNetworkAgent) { + mMockNetworkAgent.setNetworkCapabilities(mNetworkCapabilities, + false /* sendToConnectivityService */); + } + } + + public void disconnect() { + mConnected = false; + mConfig = null; + } + } + private class FakeWakeupMessage extends WakeupMessage { private static final int UNREASONABLY_LONG_WAIT = 1000; @@ -894,10 +989,12 @@ public class ConnectivityServiceTest { public void mockVpn(int uid) { synchronized (mVpns) { + int userId = UserHandle.getUserId(uid); + mMockVpn = new MockVpn(userId); // This has no effect unless the VPN is actually connected, because things like // getActiveNetworkForUidInternal call getNetworkAgentInfoForNetId on the VPN // netId, and check if that network is actually connected. - mVpns.put(UserHandle.getUserId(Process.myUid()), mMockVpn); + mVpns.put(userId, mMockVpn); } } @@ -927,7 +1024,6 @@ public class ConnectivityServiceTest { MockitoAnnotations.initMocks(this); when(mMetricsService.defaultNetworkMetrics()).thenReturn(mDefaultNetworkMetrics); - when(mMockVpn.appliesToUid(Process.myUid())).thenReturn(true); // InstrumentationTestRunner prepares a looper, but AndroidJUnitRunner does not. // http://b/25897652 . @@ -1547,7 +1643,8 @@ public class ConnectivityServiceTest { void expectCapabilitiesLike(Predicate fn, MockNetworkAgent agent) { CallbackInfo cbi = expectCallback(CallbackState.NETWORK_CAPABILITIES, agent); - assertTrue(fn.test((NetworkCapabilities) cbi.arg)); + assertTrue("Received capabilities don't match expectations : " + cbi.arg, + fn.test((NetworkCapabilities) cbi.arg)); } void assertNoCallback() { @@ -2575,9 +2672,10 @@ public class ConnectivityServiceTest { final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); final ArraySet ranges = new ArraySet<>(); ranges.add(new UidRange(uid, uid)); - when(mMockVpn.getNetId()).thenReturn(vpnNetworkAgent.getNetwork().netId); - vpnNetworkAgent.setUids(ranges); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); vpnNetworkAgent.connect(true); + mMockVpn.connect(); defaultNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); assertEquals(defaultNetworkCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); @@ -4104,9 +4202,10 @@ public class ConnectivityServiceTest { final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); final ArraySet ranges = new ArraySet<>(); ranges.add(new UidRange(uid, uid)); - when(mMockVpn.getNetId()).thenReturn(vpnNetworkAgent.getNetwork().netId); - vpnNetworkAgent.setUids(ranges); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); vpnNetworkAgent.connect(false); + mMockVpn.connect(); genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4138,7 +4237,7 @@ public class ConnectivityServiceTest { defaultCallback.expectCallback(CallbackState.NETWORK_CAPABILITIES, vpnNetworkAgent); ranges.add(new UidRange(uid, uid)); - vpnNetworkAgent.setUids(ranges); + mMockVpn.setUids(ranges); genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent); genericNotVpnNetworkCallback.assertNoCallback(); @@ -4188,9 +4287,10 @@ public class ConnectivityServiceTest { MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); final ArraySet ranges = new ArraySet<>(); ranges.add(new UidRange(uid, uid)); - when(mMockVpn.getNetId()).thenReturn(vpnNetworkAgent.getNetwork().netId); - vpnNetworkAgent.setUids(ranges); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */); + mMockVpn.connect(); defaultCallback.assertNoCallback(); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); @@ -4199,9 +4299,10 @@ public class ConnectivityServiceTest { defaultCallback.assertNoCallback(); vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - when(mMockVpn.getNetId()).thenReturn(vpnNetworkAgent.getNetwork().netId); - vpnNetworkAgent.setUids(ranges); + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); vpnNetworkAgent.connect(true /* validated */, true /* hasInternet */); + mMockVpn.connect(); defaultCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); @@ -4210,13 +4311,115 @@ public class ConnectivityServiceTest { defaultCallback.expectAvailableCallbacksValidated(mWiFiNetworkAgent); vpnNetworkAgent = new MockNetworkAgent(TRANSPORT_VPN); - when(mMockVpn.getNetId()).thenReturn(vpnNetworkAgent.getNetwork().netId); ranges.clear(); - vpnNetworkAgent.setUids(ranges); - + mMockVpn.setNetworkAgent(vpnNetworkAgent); + mMockVpn.setUids(ranges); vpnNetworkAgent.connect(false /* validated */, true /* hasInternet */); + mMockVpn.connect(); defaultCallback.assertNoCallback(); mCm.unregisterNetworkCallback(defaultCallback); } + + @Test + public void testVpnSetUnderlyingNetworks() { + final int uid = Process.myUid(); + + final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback(); + final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN) + .addTransportType(TRANSPORT_VPN) + .build(); + NetworkCapabilities nc; + mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback); + vpnNetworkCallback.assertNoCallback(); + + final MockNetworkAgent vpnNetworkAgent = new MockNetworkAgent(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 */); + + vpnNetworkCallback.expectAvailableThenValidatedCallbacks(vpnNetworkAgent); + nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork()); + assertTrue(nc.hasTransport(TRANSPORT_VPN)); + assertFalse(nc.hasTransport(TRANSPORT_CELLULAR)); + assertFalse(nc.hasTransport(TRANSPORT_WIFI)); + // For safety reasons a VPN without underlying networks is considered metered. + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED)); + + // Connect cell and use it as an underlying network. + mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR); + mCellNetworkAgent.connect(true); + + mService.setUnderlyingNetworksForVpn( + new Network[] { mCellNetworkAgent.getNetwork() }); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) + && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI); + mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); + mWiFiNetworkAgent.connect(true); + + mService.setUnderlyingNetworksForVpn( + new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork() }); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) + && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + // Don't disconnect, but note the VPN is not using wifi any more. + mService.setUnderlyingNetworksForVpn( + new Network[] { mCellNetworkAgent.getNetwork() }); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) + && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + // Use Wifi but not cell. Note the VPN is now unmetered. + mService.setUnderlyingNetworksForVpn( + new Network[] { mWiFiNetworkAgent.getNetwork() }); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) + && caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + // Use both again. + mService.setUnderlyingNetworksForVpn( + new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork() }); + + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) + && !caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + if (false) { // TODO : reactivate this ; in the current state, the below fail. + // 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.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI) + && caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + + // Disconnect wifi too. No underlying networks should mean this is now metered, + // unfortunately a discrepancy in the current implementation has this unmetered. + // TODO : fix this. + mWiFiNetworkAgent.disconnect(); + vpnNetworkCallback.expectCapabilitiesLike((caps) -> caps.hasTransport(TRANSPORT_VPN) + && !caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI) + && caps.hasCapability(NET_CAPABILITY_NOT_METERED), + vpnNetworkAgent); + } + + mMockVpn.disconnect(); + } } -- 2.11.0