Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add try/catch around trying to access the connectivityManager APIs. #160

Merged
merged 6 commits into from
Sep 27, 2021
Merged

Add try/catch around trying to access the connectivityManager APIs. #160

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
640b994
Add try/catch around trying to access the connectivityManager APIs.
Sep 27, 2021
b8f9ace
a small tweak from PR review
Sep 27, 2021
ad67cc5
catch the exception one layer higher for simplicity and clarity.
Sep 27, 2021
bc95ca6
small tweak
Sep 27, 2021
25f23ee
small tweak 2
Sep 27, 2021
c4bdcdd
remove unneeded local var
Sep 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions splunk-otel-android/src/main/java/com/splunk/rum/PostApi29NetworkDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package com.splunk.rum;

import static com.splunk.rum.ConnectionUtil.NO_NETWORK;
import static com.splunk.rum.ConnectionUtil.UNKNOWN_NETWORK;
Comment on lines +19 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea why IDEA decided to reorder these imports. 🤷🏽


import android.Manifest;
import android.annotation.SuppressLint;
import android.content.Context;
Expand All @@ -28,9 +31,6 @@
import androidx.annotation.RequiresApi;
import androidx.core.app.ActivityCompat;

import static com.splunk.rum.ConnectionUtil.NO_NETWORK;
import static com.splunk.rum.ConnectionUtil.UNKNOWN_NETWORK;

@RequiresApi(api = Build.VERSION_CODES.Q)
class PostApi29NetworkDetector implements NetworkDetector {
private final ConnectivityManager connectivityManager;
Expand All @@ -46,24 +46,30 @@ class PostApi29NetworkDetector implements NetworkDetector {
@SuppressLint("MissingPermission")
@Override
public CurrentNetwork detectCurrentNetwork() {
NetworkCapabilities capabilities = connectivityManager.getNetworkCapabilities(connectivityManager.getActiveNetwork());
if (capabilities == null) {
return NO_NETWORK;
}
String subType = null;
if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)) {
// If the app has the permission, use it to get a subtype.
if (hasPermission(Manifest.permission.READ_PHONE_STATE)) {
subType = getDataNetworkTypeName(telephonyManager.getDataNetworkType());
try {
NetworkCapabilities capabilities = connectivityManager.getNetworkCapabilities(connectivityManager.getActiveNetwork());
if (capabilities == null) {
return NO_NETWORK;
}
String subType = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this change, but looks like the subType should be moved into the if scope block.

if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)) {
// If the app has the permission, use it to get a subtype.
if (hasPermission(Manifest.permission.READ_PHONE_STATE)) {
subType = getDataNetworkTypeName(telephonyManager.getDataNetworkType());
}
return new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, subType);
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) {
return new CurrentNetwork(NetworkState.TRANSPORT_WIFI, null);
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN)) {
return new CurrentNetwork(NetworkState.TRANSPORT_VPN, null);
}
return new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, subType);
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) {
return new CurrentNetwork(NetworkState.TRANSPORT_WIFI, null);
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN)) {
return new CurrentNetwork(NetworkState.TRANSPORT_VPN, null);
//there is an active network, but it doesn't fall into the neat buckets above
return UNKNOWN_NETWORK;
} catch (Exception e) {
//guard against possible bugs/security issues with using the connectivityManager.
// see: https://issuetracker.google.com/issues/175055271
return UNKNOWN_NETWORK;
}
//there is an active network, but it doesn't fall into the neat buckets above
return UNKNOWN_NETWORK;
}

//visible for testing
Expand Down
38 changes: 22 additions & 16 deletions splunk-otel-android/src/main/java/com/splunk/rum/SimpleNetworkDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@

package com.splunk.rum;

import android.net.ConnectivityManager;
import android.net.NetworkInfo;

import static com.splunk.rum.ConnectionUtil.NO_NETWORK;
import static com.splunk.rum.ConnectionUtil.UNKNOWN_NETWORK;

import android.net.ConnectivityManager;
import android.net.NetworkInfo;

class SimpleNetworkDetector implements NetworkDetector {
private final ConnectivityManager connectivityManager;

Expand All @@ -31,20 +31,26 @@ class SimpleNetworkDetector implements NetworkDetector {

@Override
public CurrentNetwork detectCurrentNetwork() {
NetworkInfo activeNetwork = connectivityManager.getActiveNetworkInfo(); // Deprecated in API 29
if (activeNetwork == null) {
return NO_NETWORK;
}
switch (activeNetwork.getType()) {
case ConnectivityManager.TYPE_MOBILE: // Deprecated in API 28
return new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, activeNetwork.getSubtypeName());
case ConnectivityManager.TYPE_WIFI: // Deprecated in API 28
return new CurrentNetwork(NetworkState.TRANSPORT_WIFI, activeNetwork.getSubtypeName());
case ConnectivityManager.TYPE_VPN:
return new CurrentNetwork(NetworkState.TRANSPORT_VPN, activeNetwork.getSubtypeName());
try {
NetworkInfo activeNetwork = connectivityManager.getActiveNetworkInfo(); // Deprecated in API 29
if (activeNetwork == null) {
return NO_NETWORK;
}
switch (activeNetwork.getType()) {
case ConnectivityManager.TYPE_MOBILE: // Deprecated in API 28
return new CurrentNetwork(NetworkState.TRANSPORT_CELLULAR, activeNetwork.getSubtypeName());
case ConnectivityManager.TYPE_WIFI: // Deprecated in API 28
return new CurrentNetwork(NetworkState.TRANSPORT_WIFI, activeNetwork.getSubtypeName());
case ConnectivityManager.TYPE_VPN:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside the scope of this specific change, but static imports on these would be nice for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when I wrote this originally, I wanted to make sure it was clear which of these things were Android's, and which were ours, and keeping the class name prefixes made that a little more clear to me.

return new CurrentNetwork(NetworkState.TRANSPORT_VPN, activeNetwork.getSubtypeName());
}
//there is an active network, but it doesn't fall into the neat buckets above
return UNKNOWN_NETWORK;
} catch (Exception e) {
//guard against security issues/bugs when accessing the connectivityManager.
// see: https://issuetracker.google.com/issues/175055271
return UNKNOWN_NETWORK;
}
//there is an active network, but it doesn't fall into the neat buckets above
return UNKNOWN_NETWORK;
}

}
23 changes: 19 additions & 4 deletions splunk-otel-android/src/test/java/com/splunk/rum/PostApi29NetworkDetectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package com.splunk.rum;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import android.content.Context;
import android.net.ConnectivityManager;
import android.net.Network;
Expand All @@ -28,10 +32,6 @@
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@RunWith(RobolectricTestRunner.class)
@Config(sdk = Build.VERSION_CODES.Q)
public class PostApi29NetworkDetectorTest {
Expand All @@ -51,6 +51,21 @@ public void none() {
assertEquals(new CurrentNetwork(NetworkState.NO_NETWORK_AVAILABLE, null), currentNetwork);
}

@Test
public void exception() {
ConnectivityManager connectivityManager = mock(ConnectivityManager.class);
TelephonyManager telephonyManager = mock(TelephonyManager.class);
Context context = mock(Context.class);

Network network = mock(Network.class);
when(connectivityManager.getActiveNetwork()).thenReturn(network);
when(connectivityManager.getNetworkCapabilities(network)).thenThrow(new SecurityException("bug"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻


PostApi29NetworkDetector networkDetector = new PostApi29NetworkDetector(connectivityManager, telephonyManager, context);
CurrentNetwork currentNetwork = networkDetector.detectCurrentNetwork();
assertEquals(new CurrentNetwork(NetworkState.TRANSPORT_UNKNOWN, null), currentNetwork);
}

@Test
public void wifi() {
ConnectivityManager connectivityManager = mock(ConnectivityManager.class);
Expand Down
21 changes: 17 additions & 4 deletions splunk-otel-android/src/test/java/com/splunk/rum/SimpleNetworkDetectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package com.splunk.rum;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import android.content.Context;
import android.net.ConnectivityManager;
import android.net.NetworkInfo;
Expand All @@ -30,10 +34,6 @@
import org.robolectric.annotation.Config;
import org.robolectric.shadows.ShadowNetworkInfo;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@RunWith(RobolectricTestRunner.class)
@Config(sdk = Build.VERSION_CODES.P)
public class SimpleNetworkDetectorTest {
Expand All @@ -50,6 +50,19 @@ public void none() {
assertEquals(new CurrentNetwork(NetworkState.NO_NETWORK_AVAILABLE, null), currentNetwork);
}

@Test
public void exception() {
ConnectivityManager connectivityManager =
mock(ConnectivityManager.class);

when(connectivityManager.getActiveNetworkInfo()).thenThrow(new SecurityException("bug"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

SimpleNetworkDetector networkDetector = new SimpleNetworkDetector(connectivityManager);

CurrentNetwork currentNetwork = networkDetector.detectCurrentNetwork();

assertEquals(new CurrentNetwork(NetworkState.TRANSPORT_UNKNOWN, null), currentNetwork);
}

@Test
public void other() {
ConnectivityManager connectivityManager =
Expand Down