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.

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

Android detects VPN (wireguard) connection as WI-FI enabled #291

Open
Galunid opened this issue Sep 28, 2020 · 15 comments
Open

Android detects VPN (wireguard) connection as WI-FI enabled #291

Galunid opened this issue Sep 28, 2020 · 15 comments

Comments

@Galunid
Copy link
Member

Galunid commented Sep 28, 2020

  • KOReader version: v2020.09
  • Device: Xiaomi MI A2 Lite/Onyx Boox Nova 2 (should be device independent)

Issue

When wifi is disabled, and device has VPN (in my case wireguard) enabled KOReader detects wifi as enabled. It's problematic because KOSync plugin/Calibre plugin have to timeout if triggered in this state.

Steps to reproduce

Connect to wireguard -> Disconnect wifi -> KOReader still detects wifi enabled (because VPN connection is still active)

@pazos
Copy link
Member

pazos commented Sep 28, 2020

See

override fun getNetworkInfo(): String {
val connectivityManager = this.getSystemService(Context.CONNECTIVITY_SERVICE)
as ConnectivityManager
var connectionType: Int = ACTIVE_NETWORK_NONE
val connected: Boolean =
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
connectivityManager.activeNetwork?.let { net ->
connectivityManager.getNetworkCapabilities(net)?.let {
when {
it.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) -> {
connectionType = ACTIVE_NETWORK_WIFI
true
}
it.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR) -> {
connectionType = ACTIVE_NETWORK_MOBILE
true
}
it.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET) -> {
connectionType = ACTIVE_NETWORK_ETHERNET
true
}
it.hasTransport(NetworkCapabilities.TRANSPORT_BLUETOOTH) -> {
connectionType = ACTIVE_NETWORK_BLUETOOTH
true
}
it.hasTransport(NetworkCapabilities.TRANSPORT_VPN) -> {
connectionType = ACTIVE_NETWORK_VPN
true
}
else -> false
}
} ?: false
} ?: false
} else {
connectivityManager.run {
connectivityManager.activeNetworkInfo?.run {
when (type) {
ConnectivityManager.TYPE_WIFI -> {
connectionType = ACTIVE_NETWORK_WIFI
true
}
ConnectivityManager.TYPE_MOBILE -> {
connectionType = ACTIVE_NETWORK_MOBILE
true
}
ConnectivityManager.TYPE_ETHERNET -> {
connectionType = ACTIVE_NETWORK_ETHERNET
true
}
ConnectivityManager.TYPE_BLUETOOTH -> {
connectionType = ACTIVE_NETWORK_BLUETOOTH
true
}
ConnectivityManager.TYPE_VPN -> {
connectionType = ACTIVE_NETWORK_VPN
true
}
else -> false
}
}
} ?: false
}
return String.format(Locale.US, "%d;%d", if (connected) 1 else 0, connectionType)
}

It is expected. VPN will prevail over wifi (ie: if both are active). Maybe if connection type is VPN do aditional online check?.

I wouldn't want to remove VPN as a valid network type. I use gnirehtet a lot (which is recongized as VPN network).

@Galunid
Copy link
Member Author

Galunid commented Sep 28, 2020

Perhaps it's enough to just not return true or return connected in those snippets

it.hasTransport(NetworkCapabilities.TRANSPORT_VPN) -> {
connectionType = ACTIVE_NETWORK_VPN
true
}

ConnectivityManager.TYPE_VPN -> {
connectionType = ACTIVE_NETWORK_VPN
true
}

I'm not actually too clear on how the when works when multiple conditions are true, but my idea is to not change "connected" state with VPN connection, because user being connected to VPN doesn't imply internet connection. Hope it makes sense

@pazos
Copy link
Member

pazos commented Sep 28, 2020

@Galunid: the real problem is android != all the other platforms.

On embedded linux devices network == wifi
On desktop we don't care about network

On android network means a lot of things and VPN is a valid kind of network.
It's usually used over WiFi, over mobile and over Ethernet.

You might want to try

--- a/frontend/device/android/device.lua
+++ b/frontend/device/android/device.lua
@@ -259,9 +259,10 @@ function Device:initNetworkManager(NetworkMgr)
     end
 
     function NetworkMgr:isWifiOn()
-        local ok = android.getNetworkInfo()
+        local ok, transport = android.getNetworkInfo()
         ok = tonumber(ok)
         if not ok then return false end
+        if transport == C.ANETWORK_VPN then return false end
         return ok == 1
     end
 end

A snippet like that is cool for own usage, but I don't like it as is even more broken than before :)

Hence my comment

I wouldn't want to remove VPN as a valid network type. I use gnirehtet a lot (which is recongized as VPN network).

@pazos
Copy link
Member

pazos commented Sep 28, 2020

Of course the proper implementation would be

function NetworkMgr:isWifiOn()
        local ok, transport = android.getNetworkInfo()
        ok = tonumber(ok)
        if not ok then return false end
        return ok == 1 and transport == C.ANETWORK_WIFI
end

But that will break wikipedia over mobile network, among other things.

@Galunid
Copy link
Member Author

Galunid commented Sep 29, 2020

@pazos
What I had in mind is something more like this (this targets android only devices), basically extra check if WIFI is up when we detect VPN.
https://github.com/Galunid/android-luajit-launcher/blob/eb5198eac8136a34c824fd4d8bc347685fb9e377/app/src/org/koreader/launcher/BaseActivity.kt#L363-L371
It's pretty crude, but it works for me. If you want I can clean it up a bit and submit PR, if not I can keep it to myself ;-)
For older devices, I don't really know how to fix that.

EDIT: Still doesn't work with KOSync...
EDIT2: KOSync doesn't even check if network is online

@NiLuJe
Copy link
Member

NiLuJe commented Sep 29, 2020

There's a good reason for that: an online check will, by definition, be blocking.

(Remember gitter? ;p)

IIRC, it also doesn't actually check anything most of the time, because it can be triggered approximately seven billion different ways.

The one thing it tries to rely on (the NetworkConnected/NetworkDisconnected events), Android doesn't/can't support (c.f., when the Wi-Fi cleanup was implemented, we discussed that with @pazos).

@Galunid
Copy link
Member Author

Galunid commented Sep 29, 2020

It makes sense, except it doesn't. Since if network is offline, KOSync starts doing network stuff that's blocking (at least on android and kobo) and hangs device for ~30 seconds (till request timeouts). Am I missing something?

@NiLuJe
Copy link
Member

NiLuJe commented Sep 29, 2020

IIRC, it shouldn't happen on Kobo, at least in most cases. But, like I said, this thing is full of corner cases, so, who knows ;).

(Also, I don't actually use it, so I have no first hand experience with it).

@Galunid
Copy link
Member Author

Galunid commented Sep 29, 2020

It happens on Kobo when autosync is enabled and it's pretty random I didn't really have trouble when I triggered it manually. On android it causes ANR (doesn't matter if triggered manually or automatically).

@pazos
Copy link
Member

pazos commented Sep 29, 2020

What I had in mind is something more like this (this targets android only devices), basically extra check if WIFI is up when we detect VPN.

Partially cool. If active network is VPN then see in all networks if has a different transport (wifi, ethernet, bluetooth) and return that as the active network. In any cases that function should return true if an active network is found. Anything that changes the logic belongs to the frontend isWifiOn and can be setup based on user preferences.

The one thing it tries to rely on (the NetworkConnected/NetworkDisconnected events), Android doesn't/can't support (c.f., when the Wi-Fi cleanup was implemented, we discussed that with @pazos).

Still unimplemented. See koreader/koreader#6438 (comment) and koreader/koreader#6438 (comment)

On android it causes ANR (doesn't matter if triggered manually or automatically).

What causes ANR is a input event not consumed in 5 seconds. There's a workaround for that but I just implemented it for ANRs I found. See koreader/koreader#6266

@NiLuJe
Copy link
Member

NiLuJe commented Sep 29, 2020

@Galunid:

Something like that might help, but it'll probably also break stuff in fun and interesting ways in some cases? (I have a vague memory of someone trying something like that and failing horribly, but I don't recall the details...).

diff --git a/plugins/kosync.koplugin/main.lua b/plugins/kosync.koplugin/main.lua
index 6fcf4744..b56c0d59 100644
--- a/plugins/kosync.koplugin/main.lua
+++ b/plugins/kosync.koplugin/main.lua
@@ -494,6 +494,10 @@ function KOSync:updateProgress(manual)
         return
     end
 
+    if NetworkMgr:willRerunWhenOnline(function() self:updateProgress(manual) end) then
+        return
+    end
+
     local KOSyncClient = require("KOSyncClient")
     local client = KOSyncClient:new{
         custom_url = self.kosync_custom_server,
@@ -538,6 +542,10 @@ function KOSync:getProgress(manual)
         return
     end
 
+    if NetworkMgr:willRerunWhenOnline(function() self:getProgress(manual) end) then
+        return
+    end
+
     local KOSyncClient = require("KOSyncClient")
     local client = KOSyncClient:new{
         custom_url = self.kosync_custom_server,

@Galunid
Copy link
Member Author

Galunid commented Sep 29, 2020

@NiLuJe It was me in koreader/koreader#6489, which resulted in koreader/koreader#6535 ;-)
Sidenote, it we checked for manual variable, it should work (but only for updates triggered by user)

diff --git a/plugins/kosync.koplugin/main.lua b/plugins/kosync.koplugin/main.lua
index 6fcf4744..b56c0d59 100644
--- a/plugins/kosync.koplugin/main.lua
+++ b/plugins/kosync.koplugin/main.lua
@@ -494,6 +494,10 @@ function KOSync:updateProgress(manual)
         return
     end
 
+    if manual and NetworkMgr:willRerunWhenOnline(function() self:updateProgress(manual) end) then
+        return
+    end
+
     local KOSyncClient = require("KOSyncClient")
     local client = KOSyncClient:new{
         custom_url = self.kosync_custom_server,
@@ -538,6 +542,10 @@ function KOSync:getProgress(manual)
         return
     end
 
+    if manual and NetworkMgr:willRerunWhenOnline(function() self:getProgress(manual) end) then
+        return
+    end
+
     local KOSyncClient = require("KOSyncClient")
     local client = KOSyncClient:new{
         custom_url = self.kosync_custom_server,

@NiLuJe
Copy link
Member

NiLuJe commented Sep 29, 2020

Right, that'd be a start ;).

That, and allowing to tweak the period like the TODO says ;p.

@Galunid
Copy link
Member Author

Galunid commented Sep 30, 2020

@pazos ANR is still present in KOSync, not sure if you're interested in fixing that, but what koreader/koreader#6266 did was fix ANR in login/register functions, the more important ones getProgress/updateProgress, don't have your safeguard. I'm assume that's intentional to prevent reader from suddenly becoming unresponsive.

In any cases that function should return true if an active network is found. Anything that changes the logic belongs to the frontend isWifiOn and can be setup based on user preferences.

I don't really think it's possible to do this on frontend, since android will return the network type VPN, and it's not possible to check if there actually is an internet connection, so I'd be stuck with detecting VPN as either connected or disconnected, which is pretty much out of question, since I have VPN set as Always on in android settings. I could modify

return String.format(Locale.US, "%d;%d", if (connected) 1 else 0, connectionType)

to return different value if connected to VPN and online, vs connected to VPN offline, but that's probably less than ideal. Anyway, let me know your thoughts, feel free to close this issue, if you don't want to bother with this, since it probably doesn't affect too many people.

@pazos
Copy link
Member

pazos commented Sep 30, 2020

ANR is still present in KOSync, not sure if you're interested in fixing that, but what koreader/koreader#6266 did was fix ANR in login/register functions, the more important ones getProgress/updateProgress, don't have your safeguard. I'm assume that's intentional to prevent reader from suddenly becoming unresponsive.

Mmm. Nobody faced that issue. The safeguards were added to prevent koreader/koreader#6237. I would suggest to focus now on your original issue with VPNs here.

Other issues with KoSync can be fixed later.

don't really think it's possible to do this on frontend, since android will return the network type VPN, and it's not possible to check if there actually is an internet connection

Use a g_setting called "vpn_alone_is_a_valid_network" paired with https://github.com/koreader/koreader/issues/6726#issuecomment-700340138. So the backend network info will report true, VPN and the frontend will decide if isWifiOn based on Gsettings. If you make the changes in the backend I can fix the frontend, if you want.

So, the backend will return:

on wifi (with or without VPN): true, WIFI
on ethernet (""): true, ETHERNET
on bluetooth(""): true, BLUETOOTH
on VPN alone: true, VPN
on no network: false, nil

and the frontend will do:

if vpn_is_valid_network then
return ok == 1
else
return ok == 1 and transport ~= VPN
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants