Merge pull request #9139 from nextcloud/suppress-walled-network-test-on-metered-wifi

Suppress walled network check when on metered wifi

src/androidTest/java/com/nextcloud/client/network/ConnectivityServiceImplIT.kt

@@ -24,6 +24,7 @@ package com.nextcloud.client.network
 import android.accounts.AccountManager
 import android.content.Context
 import android.net.ConnectivityManager
+import android.os.Build
 import com.nextcloud.client.account.UserAccountManagerImpl
 import com.nextcloud.client.network.ConnectivityServiceImpl.GetRequestBuilder
 import com.owncloud.android.AbstractOnServerIT
@@ -44,7 +45,8 @@ class ConnectivityServiceImplIT : AbstractOnServerIT() {
             connectivityManager,
             userAccountManager,
             clientFactory,
-            requestBuilder
+            requestBuilder,
+            Build.VERSION.SDK_INT
         )
 
         assertTrue(sut.connectivity.isConnected)

src/main/java/com/nextcloud/client/network/ConnectivityServiceImpl.java

@@ -2,7 +2,7 @@
  * Nextcloud Android client application
  *
  * @author Chris Narkiewicz
- * Copyright (C) 2019 Chris Narkiewicz <hello@ezaquarii.com>
+ * Copyright (C) 2021 Chris Narkiewicz <hello@ezaquarii.com>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Affero General Public License as published by
@@ -20,7 +20,9 @@
 
 package com.nextcloud.client.network;
 
+import android.annotation.SuppressLint;
 import android.net.ConnectivityManager;
+import android.net.Network;
 import android.net.NetworkCapabilities;
 import android.net.NetworkInfo;
 
@@ -32,6 +34,7 @@ import com.nextcloud.operations.GetMethod;
 import org.apache.commons.httpclient.HttpStatus;
 
 import androidx.core.net.ConnectivityManagerCompat;
+import kotlin.jvm.functions.Function0;
 import kotlin.jvm.functions.Function1;
 
 class ConnectivityServiceImpl implements ConnectivityService {
@@ -40,6 +43,7 @@ class ConnectivityServiceImpl implements ConnectivityService {
     private final UserAccountManager accountManager;
     private final ClientFactory clientFactory;
     private final GetRequestBuilder requestBuilder;
+    private final int sdkVersion;
 
     static class GetRequestBuilder implements Function1<String, GetMethod> {
         @Override
@@ -51,17 +55,19 @@ class ConnectivityServiceImpl implements ConnectivityService {
     ConnectivityServiceImpl(ConnectivityManager platformConnectivityManager,
                             UserAccountManager accountManager,
                             ClientFactory clientFactory,
-                            GetRequestBuilder requestBuilder) {
+                            GetRequestBuilder requestBuilder,
+                            int sdkVersion) {
         this.platformConnectivityManager = platformConnectivityManager;
         this.accountManager = accountManager;
         this.clientFactory = clientFactory;
         this.requestBuilder = requestBuilder;
+        this.sdkVersion = sdkVersion;
     }
 
     @Override
     public boolean isInternetWalled() {
         Connectivity c = getConnectivity();
-        if (c.isConnected() && c.isWifi()) {
+        if (c.isConnected() && c.isWifi() && !c.isMetered()) {
 
             Server server = accountManager.getUser().getServer();
             String baseServerAddress = server.getUri().toString();
@@ -76,7 +82,6 @@ class ConnectivityServiceImpl implements ConnectivityService {
 
             // Content-Length is not available when using chunked transfer encoding, so check for -1 as well
             boolean result = !(status == HttpStatus.SC_NO_CONTENT && get.getResponseContentLength() <= 0);
-
             get.releaseConnection();
 
             return result;
@@ -96,21 +101,9 @@ class ConnectivityServiceImpl implements ConnectivityService {
 
         if (networkInfo != null) {
             boolean isConnected = networkInfo.isConnectedOrConnecting();
-
             // more detailed check
             boolean isMetered;
-            if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.M) {
+            isMetered = isNetworkMetered();
-                NetworkCapabilities networkCapabilities = platformConnectivityManager.getNetworkCapabilities(
-                    platformConnectivityManager.getActiveNetwork());
-
-                if (networkCapabilities != null) {
-                    isMetered = !networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED);
-                } else {
-                    isMetered = ConnectivityManagerCompat.isActiveNetworkMetered(platformConnectivityManager);
-                }
-            } else {
-                isMetered = ConnectivityManagerCompat.isActiveNetworkMetered(platformConnectivityManager);
-            }
             boolean isWifi = networkInfo.getType() == ConnectivityManager.TYPE_WIFI || hasNonCellularConnectivity();
             return new Connectivity(isConnected, isMetered, isWifi, null);
         } else {
@@ -118,6 +111,21 @@ class ConnectivityServiceImpl implements ConnectivityService {
         }
     }
 
+    @SuppressLint("NewApi") // false positive due to mocking
+    private boolean isNetworkMetered() {
+        if (sdkVersion >= android.os.Build.VERSION_CODES.M) {
+            final Network network = platformConnectivityManager.getActiveNetwork();
+            NetworkCapabilities networkCapabilities = platformConnectivityManager.getNetworkCapabilities(network);
+            if (networkCapabilities != null) {
+                return !networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED);
+            } else {
+                return ConnectivityManagerCompat.isActiveNetworkMetered(platformConnectivityManager);
+            }
+        } else {
+            return ConnectivityManagerCompat.isActiveNetworkMetered(platformConnectivityManager);
+        }
+    }
+
     private boolean hasNonCellularConnectivity() {
         for (NetworkInfo networkInfo : platformConnectivityManager.getAllNetworkInfo()) {
             if (networkInfo.isConnectedOrConnecting() && (networkInfo.getType() == ConnectivityManager.TYPE_WIFI ||

src/main/java/com/nextcloud/client/network/NetworkModule.java

@@ -22,6 +22,7 @@ package com.nextcloud.client.network;
 
 import android.content.Context;
 import android.net.ConnectivityManager;
+import android.os.Build;
 
 import com.nextcloud.client.account.UserAccountManager;
 
@@ -40,7 +41,8 @@ public class NetworkModule {
         return new ConnectivityServiceImpl(connectivityManager,
                                            accountManager,
                                            clientFactory,
-                                           new ConnectivityServiceImpl.GetRequestBuilder()
+                                           new ConnectivityServiceImpl.GetRequestBuilder(),
+                                           Build.VERSION.SDK_INT
         );
     }
 

src/test/java/com/nextcloud/client/network/ConnectivityServiceTest.kt

@@ -2,7 +2,7 @@
  * Nextcloud Android client application
  *
  * @author Chris Narkiewicz
- * Copyright (C) 2020 Chris Narkiewicz <hello@ezaquarii.com>
+ * Copyright (C) 2021 Chris Narkiewicz <hello@ezaquarii.com>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Affero General Public License as published by
@@ -20,7 +20,10 @@
 package com.nextcloud.client.network
 
 import android.net.ConnectivityManager
+import android.net.Network
+import android.net.NetworkCapabilities
 import android.net.NetworkInfo
+import android.os.Build
 import com.nextcloud.client.account.Server
 import com.nextcloud.client.account.User
 import com.nextcloud.client.account.UserAccountManager
@@ -28,8 +31,10 @@ import com.nextcloud.client.logger.Logger
 import com.nextcloud.common.PlainClient
 import com.nextcloud.operations.GetMethod
 import com.nhaarman.mockitokotlin2.any
+import com.nhaarman.mockitokotlin2.eq
 import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.never
+import com.nhaarman.mockitokotlin2.times
 import com.nhaarman.mockitokotlin2.verify
 import com.nhaarman.mockitokotlin2.whenever
 import com.owncloud.android.lib.resources.status.OwnCloudVersion
@@ -89,6 +94,12 @@ class ConnectivityServiceTest {
         @Mock
         lateinit var requestBuilder: ConnectivityServiceImpl.GetRequestBuilder
 
+        @Mock
+        lateinit var network: Network
+
+        @Mock
+        lateinit var networkCapabilities: NetworkCapabilities
+
         @Mock
         lateinit var logger: Logger
 
@@ -108,11 +119,16 @@ class ConnectivityServiceTest {
                 platformConnectivityManager,
                 accountManager,
                 clientFactory,
-                requestBuilder
+                requestBuilder,
+                Build.VERSION_CODES.M
             )
 
+            whenever(networkCapabilities.hasCapability(eq(NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED)))
+                .thenReturn(true)
+            whenever(platformConnectivityManager.activeNetwork).thenReturn(network)
             whenever(platformConnectivityManager.activeNetworkInfo).thenReturn(networkInfo)
             whenever(platformConnectivityManager.allNetworkInfo).thenReturn(arrayOf(networkInfo))
+            whenever(platformConnectivityManager.getNetworkCapabilities(any())).thenReturn(networkCapabilities)
             whenever(requestBuilder.invoke(any())).thenReturn(getRequest)
             whenever(clientFactory.createPlainClient()).thenReturn(client)
             whenever(user.server).thenReturn(newServer)
@@ -235,12 +251,55 @@ class ConnectivityServiceTest {
             whenever(networkInfo.isConnectedOrConnecting).thenReturn(true)
             whenever(networkInfo.type).thenReturn(ConnectivityManager.TYPE_WIFI)
             whenever(accountManager.getServerVersion(any())).thenReturn(OwnCloudVersion.nextcloud_20)
-            assertTrue(
+            connectivityService.connectivity.let {
-                "Precondition failed",
+                assertTrue(it.isConnected)
-                connectivityService.connectivity.let {
+                assertTrue(it.isWifi)
-                    it.isConnected && it.isWifi
+                assertFalse(it.isMetered)
-                }
+            }
-            )
+        }
+
+        @Test
+        fun `request not sent when not connected`() {
+            // GIVEN
+            //      network is not connected
+            whenever(networkInfo.isConnectedOrConnecting).thenReturn(false)
+            whenever(networkInfo.isConnected).thenReturn(false)
+
+            // WHEN
+            //      connectivity is checked
+            val result = connectivityService.isInternetWalled
+
+            // THEN
+            //      connection is walled
+            //      request is not sent
+            assertTrue("Server should not be accessible", result)
+            verify(requestBuilder, never()).invoke(any())
+            verify(client, never()).execute(any())
+        }
+
+        @Test
+        fun `request not sent when wifi is metered`() {
+            // GIVEN
+            //      network is connected to wifi
+            //      wifi is metered
+            whenever(networkCapabilities.hasCapability(any())).thenReturn(false) // this test is mocked for API M
+            whenever(platformConnectivityManager.isActiveNetworkMetered).thenReturn(true)
+            connectivityService.connectivity.let {
+                assertTrue("should be connected", it.isConnected)
+                assertTrue("should be connected to wifi", it.isWifi)
+                assertTrue("check mocking, this check is complicated and depends on SDK version", it.isMetered)
+            }
+
+            // WHEN
+            //      connectivity is checked
+            val result = connectivityService.isInternetWalled
+
+            // THEN
+            //      assume internet is not walled
+            //      request is not sent
+            assertFalse("Server should not be accessible", result)
+            verify(requestBuilder, never()).invoke(any())
+            verify(getRequest, never()).execute(any<PlainClient>())
         }
 
         @Test
@@ -260,7 +319,7 @@ class ConnectivityServiceTest {
             //      request is not sent
             assertTrue("Server should not be accessible", result)
             verify(requestBuilder, never()).invoke(any())
-            verify(client, never()).execute(any())
+            verify(getRequest, never()).execute(any<PlainClient>())
         }
 
         fun mockResponse(contentLength: Long = 0, status: Int = HttpStatus.SC_OK) {
@@ -274,18 +333,21 @@ class ConnectivityServiceTest {
         fun `status 204 means internet is not walled`() {
             mockResponse(contentLength = 0, status = HttpStatus.SC_NO_CONTENT)
             assertFalse(connectivityService.isInternetWalled)
+            verify(getRequest, times(1)).execute(client)
         }
 
         @Test
         fun `status 204 and no content length means internet is not walled`() {
             mockResponse(contentLength = -1, status = HttpStatus.SC_NO_CONTENT)
             assertFalse(connectivityService.isInternetWalled)
+            verify(getRequest, times(1)).execute(client)
         }
 
         @Test
         fun `other status than 204 means internet is walled`() {
             mockResponse(contentLength = 0, status = HttpStatus.SC_GONE)
             assertTrue(connectivityService.isInternetWalled)
+            verify(getRequest, times(1)).execute(client)
         }
 
         @Test
@@ -295,6 +357,7 @@ class ConnectivityServiceTest {
             val urlCaptor = ArgumentCaptor.forClass(String::class.java)
             verify(requestBuilder).invoke(urlCaptor.capture())
             assertTrue("Invalid URL used to check status", urlCaptor.value.endsWith("/index.php/204"))
+            verify(getRequest, times(1)).execute(client)
         }
     }
 }